lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1311487663
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
shouldn't `values.length` be used instead of `this.capacity`?
```suggestion
bucket = signSafeMod(keyHash, values.length);
```
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongPairSet.java:
##########
@@ -331,7 +331,11 @@ private static final class Section extends StampedLock {
boolean contains(long item1, long item2, int hash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(hash, capacity);
+
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 2 as capacity to avoid rehash changing
capacity
+ int bucket = signSafeMod(hash, table.length / 2);
Review Comment:
similar comment here about requesting the rationale for using 2 as the
divisior.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -345,7 +348,8 @@ LongPair get(long key1, long key2, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
-
+ // update local variable
+ table = this.table;
bucket = signSafeMod(keyHash, capacity);
Review Comment:
Why is capacity used in this case? (while `table.length / 4` is used
initially)
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashSet.java:
##########
@@ -321,24 +321,20 @@ boolean contains(V value, int keyHash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
```suggestion
bucket = signSafeMod(keyHash, values.length);
```
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongPairSet.java:
##########
@@ -353,6 +357,8 @@ boolean contains(long item1, long item2, int hash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ table = this.table;
bucket = signSafeMod(hash, capacity);
Review Comment:
similar comment: why use capacity instead of `table.length / 2`?
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashMap.java:
##########
@@ -332,7 +332,11 @@ private static final class Section<K, V> extends
StampedLock {
V get(K key, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+
+ // add local variable here, so OutOfBound won't happen
+ Object[] table = this.table;
+ // calculate table.length / 2 as capacity to avoid rehash changing
capacity
+ int bucket = signSafeMod(keyHash, table.length / 2);
Review Comment:
see previous comment for similar code lines
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +322,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing
capacity
+ int bucket = signSafeMod(keyHash, table.length / 4);
Review Comment:
what is the rationale behind choosing 4 as the divisior?
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashMap.java:
##########
@@ -354,6 +358,8 @@ V get(K key, int keyHash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ table = this.table;
bucket = signSafeMod(keyHash, capacity);
Review Comment:
similar comment as before
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
