Fix a bug in LongPriorityQueue internal remove (#10832)

Motivation:
We rely on this functionality in PoolChunk, and a bug was caught by a non-deterministic test failure

Modification:
Went back to the Algorithms book, and reimplemented remove() the way it was meant to.

Result:
No test failures after 200.000 runs, so we have some confidence the code is correct now.
This commit is contained in:
Chris Vest 2020-12-02 13:06:00 +01:00 committed by GitHub
parent b27f0fccce
commit d2e16fb621
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 21 additions and 11 deletions

View File

@ -36,24 +36,27 @@ final class LongPriorityQueue {
array = Arrays.copyOf(array, 1 + (array.length - 1) * 2);
}
array[size] = handle;
lift();
lift(size);
}
public void remove(long value) {
for (int i = 1; i <= size; i++) {
if (array[i] == value) {
if (i == size) {
array[i] = 0;
} else {
array[i] = array[size];
sink(i);
}
size--;
array[i] = array[size--];
lift(i);
sink(i);
return;
}
}
}
public long peek() {
if (size == 0) {
return NO_VALUE;
}
return array[1];
}
public long poll() {
if (size == 0) {
return NO_VALUE;
@ -70,8 +73,7 @@ final class LongPriorityQueue {
return size == 0;
}
private void lift() {
int index = size;
private void lift(int index) {
int parentIndex;
while (index > 1 && subord(parentIndex = index >> 1, index)) {
swap(index, parentIndex);

View File

@ -95,7 +95,7 @@ class LongPriorityQueueTest {
@Test
public void internalRemoveMustPreserveOrder() {
ThreadLocalRandom tlr = ThreadLocalRandom.current();
int initialValues = tlr.nextInt(5, 30);
int initialValues = tlr.nextInt(1, 30);
ArrayList<Long> values = new ArrayList<Long>();
LongPriorityQueue pq = new LongPriorityQueue();
for (int i = 0; i < initialValues; i++) {
@ -127,13 +127,21 @@ class LongPriorityQueueTest {
pq.offer(10);
pq.offer(6);
pq.remove(10);
assertThat(pq.peek()).isEqualTo(5);
assertThat(pq.peek()).isEqualTo(5);
assertThat(pq.poll()).isEqualTo(5);
assertThat(pq.peek()).isEqualTo(5);
assertThat(pq.poll()).isEqualTo(5);
assertThat(pq.peek()).isEqualTo(6);
assertThat(pq.poll()).isEqualTo(6);
assertThat(pq.peek()).isEqualTo(6);
assertThat(pq.peek()).isEqualTo(6);
assertThat(pq.poll()).isEqualTo(6);
assertThat(pq.peek()).isEqualTo(10);
assertThat(pq.poll()).isEqualTo(10);
assertThat(pq.poll()).isEqualTo(10);
assertTrue(pq.isEmpty());
assertThat(pq.poll()).isEqualTo(LongPriorityQueue.NO_VALUE);
assertThat(pq.peek()).isEqualTo(LongPriorityQueue.NO_VALUE);
}
}