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 11:49:39 UTC
[GitHub] [pulsar] lhotari opened a new pull request #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
lhotari opened a new pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678
### Motivation
PositionImpl's toString, compareTo and hashCode methods aren't consistent with the style of performance optimized code used in Pulsar in a lot of hotspots.
- `String.format` should be avoided in any performance optimized code.
- `compareTo` was using Guava's ComparisonChain which creates object instances. It's better to avoid it in this case.
- `Objects.hashCode` creates a new array instance on each call. that can be avoided.
### Modifications
- refactor `toString`, `compareTo` and `hashCode` methods to minimize object allocations
--
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] Shawyeok edited a comment on pull request #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
Shawyeok edited a comment on pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678#issuecomment-1067486678
Looks Good To Me
--
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 #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678#discussion_r825962411
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java
##########
@@ -102,20 +99,27 @@ public PositionImpl getNext() {
*/
@Override
public String toString() {
- return String.format("%d:%d", ledgerId, entryId);
+ return ledgerId + ":" + entryId;
}
@Override
- public int compareTo(PositionImpl other) {
- checkNotNull(other);
+ public int compareTo(PositionImpl that) {
+ if (this.ledgerId != that.ledgerId) {
+ return (this.ledgerId < that.ledgerId ? -1 : 1);
+ }
+
+ if (this.entryId != that.entryId) {
+ return (this.entryId < that.entryId ? -1 : 1);
+ }
- return ComparisonChain.start().compare(this.ledgerId, other.ledgerId).compare(this.entryId, other.entryId)
- .result();
+ return 0;
}
@Override
public int hashCode() {
- return Objects.hashCode(ledgerId, entryId);
+ int result = (int) (ledgerId ^ (ledgerId >>> 32));
+ result = 31 * result + (int) (entryId ^ (entryId >>> 32));
+ return result;
Review comment:
I intentionally didn't add that since this PR is a refactoring. Surprising things could happen if it would be added as part of the hash code. It's not a very good design to have the ackSet as part of the PositionImpl in the first place. The mutable design of PositionImpl is also questionable.
--
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 #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678#discussion_r825959977
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java
##########
@@ -102,20 +99,27 @@ public PositionImpl getNext() {
*/
@Override
public String toString() {
- return String.format("%d:%d", ledgerId, entryId);
+ return ledgerId + ":" + entryId;
}
@Override
- public int compareTo(PositionImpl other) {
- checkNotNull(other);
+ public int compareTo(PositionImpl that) {
+ if (this.ledgerId != that.ledgerId) {
+ return (this.ledgerId < that.ledgerId ? -1 : 1);
Review comment:
It's not equivalent, I replied in https://github.com/apache/pulsar/pull/14679#discussion_r825958570
--
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 merged pull request #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678
--
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 #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678#discussion_r825936547
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java
##########
@@ -102,20 +99,27 @@ public PositionImpl getNext() {
*/
@Override
public String toString() {
- return String.format("%d:%d", ledgerId, entryId);
+ return ledgerId + ":" + entryId;
}
@Override
- public int compareTo(PositionImpl other) {
- checkNotNull(other);
+ public int compareTo(PositionImpl that) {
+ if (this.ledgerId != that.ledgerId) {
+ return (this.ledgerId < that.ledgerId ? -1 : 1);
Review comment:
`java.lang.Long#compare(this.ledgerId, other.ledgerId)` is more readable.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java
##########
@@ -102,20 +99,27 @@ public PositionImpl getNext() {
*/
@Override
public String toString() {
- return String.format("%d:%d", ledgerId, entryId);
+ return ledgerId + ":" + entryId;
}
@Override
- public int compareTo(PositionImpl other) {
- checkNotNull(other);
+ public int compareTo(PositionImpl that) {
+ if (this.ledgerId != that.ledgerId) {
+ return (this.ledgerId < that.ledgerId ? -1 : 1);
+ }
+
+ if (this.entryId != that.entryId) {
+ return (this.entryId < that.entryId ? -1 : 1);
+ }
- return ComparisonChain.start().compare(this.ledgerId, other.ledgerId).compare(this.entryId, other.entryId)
- .result();
+ return 0;
}
@Override
public int hashCode() {
- return Objects.hashCode(ledgerId, entryId);
+ int result = (int) (ledgerId ^ (ledgerId >>> 32));
+ result = 31 * result + (int) (entryId ^ (entryId >>> 32));
+ return result;
Review comment:
I am curious if we drop `ackSet` from hashCode on purpose or not.
--
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 #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678#discussion_r826536589
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java
##########
@@ -102,20 +99,27 @@ public PositionImpl getNext() {
*/
@Override
public String toString() {
- return String.format("%d:%d", ledgerId, entryId);
+ return ledgerId + ":" + entryId;
}
@Override
- public int compareTo(PositionImpl other) {
- checkNotNull(other);
+ public int compareTo(PositionImpl that) {
+ if (this.ledgerId != that.ledgerId) {
+ return (this.ledgerId < that.ledgerId ? -1 : 1);
+ }
+
+ if (this.entryId != that.entryId) {
+ return (this.entryId < that.entryId ? -1 : 1);
+ }
- return ComparisonChain.start().compare(this.ledgerId, other.ledgerId).compare(this.entryId, other.entryId)
- .result();
+ return 0;
}
@Override
public int hashCode() {
- return Objects.hashCode(ledgerId, entryId);
+ int result = (int) (ledgerId ^ (ledgerId >>> 32));
+ result = 31 * result + (int) (entryId ^ (entryId >>> 32));
+ return result;
Review comment:
> Surprising things could happen if it would be added as part of the hash code.
I looked into this a bit. It turns out that it can't be added into `hashcode`. Related method is
`org.apache.pulsar.broker.transaction.pendingack.impl.PendingAckHandleImpl#syncBatchPositionAckSetForTransaction`.
--
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] Shawyeok commented on pull request #14678: [Performance] Optimize PositionImpl toString, compareTo and hashCode methods
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on pull request #14678:
URL: https://github.com/apache/pulsar/pull/14678#issuecomment-1067486598
--
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