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 Norman Maurer
parent 28ef4d18f9
commit d660706588
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 = Arrays.copyOf(array, 1 + (array.length - 1) * 2);
} }
array[size] = handle; array[size] = handle;
lift(); lift(size);
} }
public void remove(long value) { public void remove(long value) {
for (int i = 1; i <= size; i++) { for (int i = 1; i <= size; i++) {
if (array[i] == value) { if (array[i] == value) {
if (i == size) { array[i] = array[size--];
array[i] = 0; lift(i);
} else { sink(i);
array[i] = array[size];
sink(i);
}
size--;
return; return;
} }
} }
} }
public long peek() {
if (size == 0) {
return NO_VALUE;
}
return array[1];
}
public long poll() { public long poll() {
if (size == 0) { if (size == 0) {
return NO_VALUE; return NO_VALUE;
@ -70,8 +73,7 @@ final class LongPriorityQueue {
return size == 0; return size == 0;
} }
private void lift() { private void lift(int index) {
int index = size;
int parentIndex; int parentIndex;
while (index > 1 && subord(parentIndex = index >> 1, index)) { while (index > 1 && subord(parentIndex = index >> 1, index)) {
swap(index, parentIndex); swap(index, parentIndex);

View File

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