From d0847f251956aaeb7898e4fb746aab64cd741785 Mon Sep 17 00:00:00 2001 From: Filipp Zhinkin Date: Thu, 6 Apr 2023 16:12:28 +0200 Subject: [PATCH] KT-57617 Delegate iterators of reversed list views to underlying list iterators Improve the performance of reversed list view iterators by delegating to underlying list iterators instead of using implementations provided by the AbstractList. Latter use AbstractList::get(index: Int) to implement next() and previous() methods and depending on the underlying list's implementation it may lead to worse performance. The change also improves reversed list views test coverage. --- .../src/kotlin/collections/ReversedViews.kt | 38 +++- .../test/collections/ReversedViewsTest.kt | 197 +++++++++++++++--- 2 files changed, 202 insertions(+), 33 deletions(-) diff --git a/libraries/stdlib/src/kotlin/collections/ReversedViews.kt b/libraries/stdlib/src/kotlin/collections/ReversedViews.kt index 3c0ba5d7208..1f4a985fddc 100644 --- a/libraries/stdlib/src/kotlin/collections/ReversedViews.kt +++ b/libraries/stdlib/src/kotlin/collections/ReversedViews.kt @@ -1,5 +1,5 @@ /* - * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors. * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. */ @@ -11,6 +11,19 @@ package kotlin.collections private open class ReversedListReadOnly(private val delegate: List) : AbstractList() { override val size: Int get() = delegate.size override fun get(index: Int): T = delegate[reverseElementIndex(index)] + + override fun iterator(): Iterator = listIterator(0) + override fun listIterator(): ListIterator = listIterator(0) + + override fun listIterator(index: Int): ListIterator = object : ListIterator { + val delegateIterator = delegate.listIterator(reversePositionIndex(index)) + override fun hasNext(): Boolean = delegateIterator.hasPrevious() + override fun hasPrevious(): Boolean = delegateIterator.hasNext() + override fun next(): T = delegateIterator.previous() + override fun nextIndex(): Int = reverseIteratorIndex(delegateIterator.previousIndex()) + override fun previous(): T = delegateIterator.next() + override fun previousIndex(): Int = reverseIteratorIndex(delegateIterator.nextIndex()) + } } private class ReversedList(private val delegate: MutableList) : AbstractMutableList() { @@ -24,6 +37,28 @@ private class ReversedList(private val delegate: MutableList) : AbstractMu override fun add(index: Int, element: T) { delegate.add(reversePositionIndex(index), element) } + + override fun iterator(): MutableIterator = listIterator(0) + override fun listIterator(): MutableListIterator = listIterator(0) + + override fun listIterator(index: Int): MutableListIterator = object : MutableListIterator { + val delegateIterator = delegate.listIterator(reversePositionIndex(index)) + override fun hasNext(): Boolean = delegateIterator.hasPrevious() + override fun hasPrevious(): Boolean = delegateIterator.hasNext() + override fun next(): T = delegateIterator.previous() + override fun nextIndex(): Int = reverseIteratorIndex(delegateIterator.previousIndex()) + override fun previous(): T = delegateIterator.next() + override fun previousIndex(): Int = reverseIteratorIndex(delegateIterator.nextIndex()) + override fun add(element: T) { + delegateIterator.add(element) + // After an insertion previous() will return an inserted element. + // Moving a cursor back by one element to return a correct value from next(). + delegateIterator.previous() + } + + override fun remove() = delegateIterator.remove() + override fun set(element: T) = delegateIterator.set(element) + } } private fun List<*>.reverseElementIndex(index: Int) = @@ -32,6 +67,7 @@ private fun List<*>.reverseElementIndex(index: Int) = private fun List<*>.reversePositionIndex(index: Int) = if (index in 0..size) size - index else throw IndexOutOfBoundsException("Position index $index must be in range [${0..size}].") +private fun List<*>.reverseIteratorIndex(index: Int) = lastIndex - index /** * Returns a reversed read-only view of the original List. diff --git a/libraries/stdlib/test/collections/ReversedViewsTest.kt b/libraries/stdlib/test/collections/ReversedViewsTest.kt index f9584a9e87a..cd79e1f9698 100644 --- a/libraries/stdlib/test/collections/ReversedViewsTest.kt +++ b/libraries/stdlib/test/collections/ReversedViewsTest.kt @@ -1,12 +1,11 @@ /* - * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors. * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. */ package test.collections import test.collections.behaviors.listBehavior -import test.collections.compare import kotlin.test.* class ReversedViewsTest { @@ -23,6 +22,14 @@ class ReversedViewsTest { } } + @Test fun testMutableBehavior() { + val original = mutableListOf(2L, 3L, Long.MAX_VALUE) + val reversed = original.reversed() + compare(reversed, original.asReversed()) { + listBehavior() + } + } + @Test fun testSimple() { assertEquals(listOf(3, 2, 1), listOf(1, 2, 3).asReversed()) assertEquals(listOf(3, 2, 1), listOf(1, 2, 3).asReversed().toList()) @@ -147,11 +154,6 @@ class ReversedViewsTest { assertTrue { 1 in mutableListOf(1, 2, 3).asReversed() } } - @Test fun testIndexOf() { - assertEquals(2, listOf(1, 2, 3).asReversed().indexOf(1)) - assertEquals(2, mutableListOf(1, 2, 3).asReversed().indexOf(1)) - } - @Test fun testBidirectionalModifications() { val original = mutableListOf(1, 2, 3, 4) val reversed = original.asReversed() @@ -165,47 +167,178 @@ class ReversedViewsTest { assertEquals(listOf(3, 2), reversed) } - @Test fun testGetIOOB() { - val success = try { - listOf(1, 2, 3).asReversed().get(3) - true - } catch (expected: IndexOutOfBoundsException) { - false + @Test fun testIndexOf() { + assertEquals(2, listOf(1, 2, 3).asReversed().indexOf(1)) + assertEquals(2, mutableListOf(1, 2, 3).asReversed().indexOf(1)) + + assertEquals(-1, listOf(1, 2, 3).asReversed().indexOf(0)) + assertEquals(-1, mutableListOf(1, 2, 3).asReversed().indexOf(0)) + } + + @Test fun testLastIndexOf() { + assertEquals(2, listOf(1, 2, 3).asReversed().indexOf(1)) + assertEquals(2, mutableListOf(1, 2, 3).asReversed().indexOf(1)) + + assertEquals(-1, listOf(1, 2, 3).asReversed().lastIndexOf(0)) + assertEquals(-1, mutableListOf(1, 2, 3).asReversed().lastIndexOf(0)) + } + + @Test fun testIteratorAdd() { + val original = mutableListOf(1, 2, 4) + val reversedView = original.asReversed() + val iter = reversedView.listIterator() + + val reversedCopy = original.reversed().toMutableList() + val copyIter = reversedCopy.listIterator() + + compare(copyIter, iter) { + propertyEquals { add(5) } + propertyEquals { previousIndex() } + propertyEquals { nextIndex() } + propertyEquals { next() } + } + assertEquals(reversedCopy, reversedView) + assertEquals(listOf(1, 2, 4, 5), original) + + compare(copyIter, iter) { + propertyEquals { add(3) } + propertyEquals { previousIndex() } + propertyEquals { nextIndex() } + propertyEquals { previous() } } - assertFalse(success) + iter.seekEnd() + iter.add(0) + assertEquals(listOf(5, 4, 3, 2, 1, 0), reversedView) + assertEquals(listOf(0, 1, 2, 3, 4, 5), original) + } + + @Test fun testIteratorRemove() { + val original = mutableListOf(0, 1, 2, 3, 4) + val reversedView = original.asReversed() + val iter = reversedView.listIterator() + + val reversedCopy = original.reversed().toMutableList() + val copyIter = reversedCopy.listIterator() + + compare(copyIter, iter) { + propertyFailsWith { remove() } + propertyEquals { + next() + remove() + } + propertyEquals { previousIndex() } + propertyEquals { nextIndex() } + propertyFailsWith { remove() } + propertyEquals { next() } + } + assertEquals(reversedCopy, reversedView) + assertEquals(listOf(0, 1, 2, 3), original) + + compare(copyIter, iter) { + propertyEquals { + next() + remove() + } + propertyEquals { previousIndex() } + propertyEquals { nextIndex() } + propertyEquals { previous() } + } + assertEquals(reversedCopy, reversedView) + assertEquals(listOf(0, 1, 3), original) + + iter.seekEnd() + iter.remove() + assertEquals(listOf(3, 1), reversedView) + assertEquals(listOf(1, 3), original) + } + + @Test fun testIteratorSet() { + val original = mutableListOf(2, 3, 4) + val iter = original.asReversed().listIterator() + + while (iter.hasNext()) { + val v = iter.next() + iter.set(v * v) + } + + assertEquals(listOf(4, 9, 16), original) + } + + @Test fun testGetIOOB() { + assertFailsWith { + listOf(1, 2, 3).asReversed().get(3) + }.let { exception -> + // we actually don't care about the exact wording, + // we just need the index to be not confusing + assertContains(exception.message!!, "index 3") + } + + assertFailsWith { + mutableListOf(1, 2, 3).asReversed().get(3) + }.let { exception -> + // we actually don't care about the exact wording, + // we just need the index to be not confusing + assertContains(exception.message!!, "index 3") + } } @Test fun testSetIOOB() { - val success = try { - mutableListOf(1, 2, 3).asReversed().set(3, 0) - true - } catch(expected: IndexOutOfBoundsException) { - false + assertFailsWith { + mutableListOf(1, 2, 3, 4).asReversed().set(4, 0) + }.let { exception -> + // we actually don't care about the exact wording, + // we just need the index to be not confusing + assertContains(exception.message!!, "index 4") } - - assertFalse(success) } @Test fun testAddIOOB() { - val success = try { + assertFailsWith { mutableListOf(1, 2, 3).asReversed().add(4, 0) - true - } catch(expected: IndexOutOfBoundsException) { - false + }.let { exception -> + // we actually don't care about the exact wording, + // we just need the index to be not confusing + assertContains(exception.message!!, "index 4") } - - assertFalse(success) } @Test fun testRemoveIOOB() { - val success = try { + assertFailsWith { mutableListOf(1, 2, 3).asReversed().removeAt(3) - true - } catch(expected: IndexOutOfBoundsException) { - false + }.let { exception -> + // we actually don't care about the exact wording, + // we just need the index to be not confusing + assertContains(exception.message!!, "index 3") + } + } + + @Test fun testIteratorNSEOnNext() { + assertFailsWith { + val it = listOf(1, 2, 3).asReversed().iterator() + it.seekEnd() + it.next() } - assertFalse(success) + assertFailsWith { + val it = mutableListOf(1, 2, 3).asReversed().iterator() + it.seekEnd() + it.next() + } + } + + @Test fun testIteratorNSEOnPrevious() { + assertFailsWith { + listOf(1, 2, 3).asReversed().listIterator().previous() + } + assertFailsWith { + mutableListOf(1, 2, 3).asReversed().listIterator().previous() + } + } +} + +private fun Iterator<*>.seekEnd() { + while (hasNext()) { + next() } }