Skip to content
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.

Sign up for GitHub

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

Sync BitList for all the operations #14057

Merged
merged 3 commits into from
May 6, 2024
Merged

Sync BitList for all the operations #14057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1aa1d93
Sync BitList for all the operations
AlbumenJ Apr 10, 2024
986a302
Fix style
AlbumenJ Apr 10, 2024
507fe77
Merge branch '3.2' into 0410-bitlist
AlbumenJ Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 37 additions & 37 deletions dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/state/BitList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,23 +93,23 @@ public BitList(List<E> originList, BitSet rootSet, List<E> tailList) {
}

// Provided by BitList only
public List<E> getOriginList() {
public synchronized List<E> getOriginList() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read behavior doesn't need to lock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use a read and write lock?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, reading without locking may lead to consistency issues.
@He-Pin If the operation itself is very short, using a read-write lock won't increase throughput. In fact, due to the internal complexity of the lock, it might even slow down. I've tested this with examples before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, at least at this case, this is not needed:)

return originList;
}

public void addIndex(int index) {
public synchronized void addIndex(int index) {
this.rootSet.set(index);
}

public int totalSetSize() {
public synchronized int totalSetSize() {
return this.originList.size();
}

public boolean indexExist(int index) {
public synchronized boolean indexExist(int index) {
return this.rootSet.get(index);
}

public E getByIndex(int index) {
public synchronized E getByIndex(int index) {
return this.originList.get(index);
}

Expand All @@ -120,36 +120,36 @@ public E getByIndex(int index) {
* @param target target bitList
* @return this bitList only contains those elements contain in both two list and source bitList's tailList
*/
public BitList<E> and(BitList<E> target) {
public synchronized BitList<E> and(BitList<E> target) {
rootSet.and(target.rootSet);
if (target.getTailList() != null) {
target.getTailList().forEach(this::addToTailList);
}
return this;
}

public BitList<E> or(BitList<E> target) {
public synchronized BitList<E> or(BitList<E> target) {
BitSet resultSet = (BitSet) rootSet.clone();
resultSet.or(target.rootSet);
return new BitList<>(originList, resultSet, tailList);
}

public boolean hasMoreElementInTailList() {
public synchronized boolean hasMoreElementInTailList() {
return CollectionUtils.isNotEmpty(tailList);
}

public List<E> getTailList() {
public synchronized List<E> getTailList() {
return tailList;
}

public void addToTailList(E e) {
public synchronized void addToTailList(E e) {
if (tailList == null) {
tailList = new LinkedList<>();
}
tailList.add(e);
}

public E randomSelectOne() {
public synchronized E randomSelectOne() {
int originSize = originList.size();
int tailSize = tailList != null ? tailList.size() : 0;
int totalSize = originSize + tailSize;
Expand Down Expand Up @@ -181,18 +181,18 @@ public static <T> BitList<T> emptyList() {

// Provided by JDK List interface
@Override
public int size() {
public synchronized int size() {
return rootSet.cardinality() + (CollectionUtils.isNotEmpty(tailList) ? tailList.size() : 0);
}

@Override
public boolean contains(Object o) {
public synchronized boolean contains(Object o) {
int idx = originList.indexOf(o);
return (idx >= 0 && rootSet.get(idx)) || (CollectionUtils.isNotEmpty(tailList) && tailList.contains(o));
}

@Override
public Iterator<E> iterator() {
public synchronized Iterator<E> iterator() {
return new BitListIterator<>(this, 0);
}

Expand All @@ -205,7 +205,7 @@ public Iterator<E> iterator() {
* Notice: It is not recommended adding duplicated element.
*/
@Override
public boolean add(E e) {
public synchronized boolean add(E e) {
int index = originList.indexOf(e);
if (index > -1) {
rootSet.set(index);
Expand All @@ -225,7 +225,7 @@ public boolean add(E e) {
* If the element is not contained in originList, try to remove from tailList.
*/
@Override
public boolean remove(Object o) {
public synchronized boolean remove(Object o) {
int idx = originList.indexOf(o);
if (idx > -1 && rootSet.get(idx)) {
rootSet.set(idx, false);
Expand All @@ -242,7 +242,7 @@ public boolean remove(Object o) {
* This may change the default behaviour when adding new element later.
*/
@Override
public void clear() {
public synchronized void clear() {
rootSet.clear();
// to remove references
originList = Collections.emptyList();
Expand All @@ -252,7 +252,7 @@ public void clear() {
}

@Override
public E get(int index) {
public synchronized E get(int index) {
int bitIndex = -1;
if (index < 0) {
throw new IndexOutOfBoundsException();
Expand All @@ -272,7 +272,7 @@ public E get(int index) {
}

@Override
public E remove(int index) {
public synchronized E remove(int index) {
int bitIndex = -1;
if (index >= rootSet.cardinality()) {
if (CollectionUtils.isNotEmpty(tailList)) {
Expand All @@ -290,7 +290,7 @@ public E remove(int index) {
}

@Override
public int indexOf(Object o) {
public synchronized int indexOf(Object o) {
int bitIndex = -1;
for (int i = 0; i < rootSet.cardinality(); i++) {
bitIndex = rootSet.nextSetBit(bitIndex + 1);
Expand All @@ -311,7 +311,7 @@ public int indexOf(Object o) {

@Override
@SuppressWarnings("unchecked")
public boolean addAll(Collection<? extends E> c) {
public synchronized boolean addAll(Collection<? extends E> c) {
if (c instanceof BitList) {
rootSet.or(((BitList<? extends E>) c).rootSet);
if (((BitList<? extends E>) c).hasMoreElementInTailList()) {
Expand All @@ -325,7 +325,7 @@ public boolean addAll(Collection<? extends E> c) {
}

@Override
public int lastIndexOf(Object o) {
public synchronized int lastIndexOf(Object o) {
int bitIndex = -1;
int index = -1;
if (CollectionUtils.isNotEmpty(tailList)) {
Expand All @@ -344,22 +344,22 @@ public int lastIndexOf(Object o) {
}

@Override
public boolean isEmpty() {
public synchronized boolean isEmpty() {
return this.rootSet.isEmpty() && CollectionUtils.isEmpty(tailList);
}

@Override
public ListIterator<E> listIterator() {
public synchronized ListIterator<E> listIterator() {
return new BitListIterator<>(this, 0);
}

@Override
public ListIterator<E> listIterator(int index) {
public synchronized ListIterator<E> listIterator(int index) {
return new BitListIterator<>(this, index);
}

@Override
public BitList<E> subList(int fromIndex, int toIndex) {
public synchronized BitList<E> subList(int fromIndex, int toIndex) {
BitSet resultSet = (BitSet) rootSet.clone();
List<E> copiedTailList = tailList == null ? null : new LinkedList<>(tailList);
if (toIndex < size()) {
Expand Down Expand Up @@ -414,7 +414,7 @@ public BitListIterator(BitList<E> bitList, int index) {
}

@Override
public boolean hasNext() {
public synchronized boolean hasNext() {
if (isInTailList) {
return tailListIterator.hasNext();
} else {
Expand All @@ -428,7 +428,7 @@ public boolean hasNext() {
}

@Override
public E next() {
public synchronized E next() {
if (isInTailList) {
if (tailListIterator.hasNext()) {
index += 1;
Expand Down Expand Up @@ -457,7 +457,7 @@ public E next() {
}

@Override
public boolean hasPrevious() {
public synchronized boolean hasPrevious() {
if (isInTailList) {
boolean hasPreviousInTailList = tailListIterator.hasPrevious();
if (hasPreviousInTailList) {
Expand All @@ -471,7 +471,7 @@ public boolean hasPrevious() {
}

@Override
public E previous() {
public synchronized E previous() {
if (isInTailList) {
boolean hasPreviousInTailList = tailListIterator.hasPrevious();
if (hasPreviousInTailList) {
Expand Down Expand Up @@ -503,17 +503,17 @@ public E previous() {
}

@Override
public int nextIndex() {
public synchronized int nextIndex() {
return hasNext() ? index + 1 : index;
}

@Override
public int previousIndex() {
public synchronized int previousIndex() {
return index;
}

@Override
public void remove() {
public synchronized void remove() {
if (lastReturnedIndex == -1) {
throw new IllegalStateException();
} else {
Expand All @@ -533,17 +533,17 @@ public void remove() {
}

@Override
public void set(E e) {
public synchronized void set(E e) {
throw new UnsupportedOperationException("Set method is not supported in BitListIterator!");
}

@Override
public void add(E e) {
public synchronized void add(E e) {
throw new UnsupportedOperationException("Add method is not supported in BitListIterator!");
}
}

public ArrayList<E> cloneToArrayList() {
public synchronized ArrayList<E> cloneToArrayList() {
if (rootSet.cardinality() == originList.size() && (CollectionUtils.isEmpty(tailList))) {
return new ArrayList<>(originList);
}
Expand All @@ -553,7 +553,7 @@ public ArrayList<E> cloneToArrayList() {
}

@Override
public BitList<E> clone() {
public synchronized BitList<E> clone() {
return new BitList<>(
originList, (BitSet) rootSet.clone(), tailList == null ? null : new LinkedList<>(tailList));
}
Expand Down
42 changes: 42 additions & 0 deletions dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/router/state/BitListTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicReference;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -576,4 +578,44 @@ void testClone2() {
set.add(new LinkedList<>(Arrays.asList("A", "B", "C", "D", "E", "F", "G")));
Assertions.assertEquals(2, set.size());
}

@Test
void testConcurrent() throws InterruptedException {
for (int i = 0; i < 100000; i++) {
BitList<String> bitList = new BitList<>(Collections.singletonList("test"));
bitList.remove("test");

CountDownLatch countDownLatch = new CountDownLatch(1);
CountDownLatch countDownLatch2 = new CountDownLatch(2);

Thread thread1 = new Thread(() -> {
try {
countDownLatch.await();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
bitList.add("test");
countDownLatch2.countDown();
});

AtomicReference<BitList<String>> ref = new AtomicReference<>();
Thread thread2 = new Thread(() -> {
try {
countDownLatch.await();
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
ref.set(bitList.clone());
countDownLatch2.countDown();
});

thread1.start();
thread2.start();

countDownLatch.countDown();
countDownLatch2.await();

Assertions.assertDoesNotThrow(() -> ref.get().iterator().hasNext());
}
}
}
Loading