From 321386d95ed7c6b1eaee8ee5e32f998d561572fc Mon Sep 17 00:00:00 2001 From: Ilya Matveev Date: Fri, 14 Apr 2017 12:50:22 +0700 Subject: [PATCH] stdlib: Improve ArrayList.removeAll implementation. This patch replaces ArrayList.removeAll implementation with a faster one. It also adds the AbstractMutableCollection class containing default implementations for addAll, removeAll and retainAll methods and fixes tests to check if removeAll removes all occurrences found or not. --- backend.native/tests/build.gradle | 5 ++ .../AbstractMutableCollectionRemoveAll.kt | 70 +++++++++++++++++++ .../tests/runtime/collections/array_list1.kt | 10 +-- .../tests/runtime/collections/hash_set0.kt | 4 +- .../kotlin/collections/AbstractCollection.kt | 49 +++++++++++++ .../kotlin/kotlin/collections/ArrayList.kt | 21 ++---- .../main/kotlin/kotlin/collections/HashSet.kt | 26 +------ 7 files changed, 139 insertions(+), 46 deletions(-) create mode 100644 backend.native/tests/runtime/collections/AbstractMutableCollectionRemoveAll.kt diff --git a/backend.native/tests/build.gradle b/backend.native/tests/build.gradle index 90cdb2e9da4..be1e640871a 100644 --- a/backend.native/tests/build.gradle +++ b/backend.native/tests/build.gradle @@ -790,6 +790,11 @@ task lateinit_notInitialized(type: RunKonanTest) { source = "codegen/lateinit/notInitialized.kt" } +task AbstractMutableCollectionRemoveAll(type: RunKonanTest) { + expectedExitStatus = 0 + source = "runtime/collections/AbstractMutableCollectionRemoveAll.kt" +} + task array0(type: RunKonanTest) { goldValue = "5\n6\n7\n8\n9\n10\n11\n12\n13\n" source = "runtime/collections/array0.kt" diff --git a/backend.native/tests/runtime/collections/AbstractMutableCollectionRemoveAll.kt b/backend.native/tests/runtime/collections/AbstractMutableCollectionRemoveAll.kt new file mode 100644 index 00000000000..c91a7353076 --- /dev/null +++ b/backend.native/tests/runtime/collections/AbstractMutableCollectionRemoveAll.kt @@ -0,0 +1,70 @@ +class TestCollection(): AbstractMutableCollection() { + companion object { + const val SIZE = 7 + } + + private val array = IntArray(SIZE) + private var len = 0 + + override val size: Int + get() = len + + override fun add(element: Int): Boolean { + if (len >= SIZE) return false + array[len++] = element + return true + } + + override fun remove(element: Int): Boolean { + val it = iterator() + while(it.hasNext()) { + if (it.next() == element) { + it.remove() + return true + } + } + return false + } + + override fun iterator(): MutableIterator = object: MutableIterator { + var nextIndex = 0 + + override fun hasNext() = nextIndex < len + override fun next() = array[nextIndex++] + + override fun remove() { + if (nextIndex == 0) throw IllegalStateException() + for (i in nextIndex..len - 1) { + array[i - 1] = array[i] + } + len-- + nextIndex-- + } + } + + override fun clear() { + len = 0 + } + +} + +fun assertEquals(a: TestCollection, b: List) { + if (a.size != b.size) { + throw AssertionError() + } + val aIt = a.iterator() + val bIt = b.iterator() + while (aIt.hasNext()) { + if (aIt.next() != bIt.next()) throw AssertionError("TestCollection contains wrong elements. Expected: $b, actual: $a.") + } +} + +fun main(args: Array) { + val c = TestCollection() + if (!c.addAll(listOf(1, 2, 3, 2, 4, 5, 4))) throw AssertionError("addAll is false when it must be true.") + if (c.addAll(listOf(1, 2)) != false) throw AssertionError("addAll is true when it must be false.") + c.removeAll(listOf(1, 2)) + assertEquals(c, listOf(3, 4, 5, 4)) + c.retainAll(listOf(4, 5)) + assertEquals(c, listOf(4, 5, 4)) +} \ No newline at end of file diff --git a/backend.native/tests/runtime/collections/array_list1.kt b/backend.native/tests/runtime/collections/array_list1.kt index 945d756c621..e92e73781f7 100644 --- a/backend.native/tests/runtime/collections/array_list1.kt +++ b/backend.native/tests/runtime/collections/array_list1.kt @@ -85,9 +85,9 @@ fun testRemove() { } fun testRemoveAll() { - val a = ArrayList(listOf("1", "2", "3", "4", "5")) + val a = ArrayList(listOf("1", "2", "3", "4", "5", "1")) assertFalse(a.removeAll(listOf("6", "7", "8"))) - assertEquals(listOf("1", "2", "3", "4", "5"), a) + assertEquals(listOf("1", "2", "3", "4", "5", "1"), a) assertTrue(a.removeAll(listOf("5", "3", "1"))) assertEquals(listOf("2", "4"), a) } @@ -277,9 +277,9 @@ fun testSubListSubListRemoveAt() { } fun testSubListRemoveAll() { - val a = ArrayList(listOf("1", "2", "3", "4", "5")) - val s = a.subList(1, 4) - assertEquals(listOf("2", "3", "4"), s) + val a = ArrayList(listOf("1", "2", "3", "3", "4", "5")) + val s = a.subList(1, 5) + assertEquals(listOf("2", "3", "3", "4"), s) assertTrue(s.removeAll(listOf("3", "5"))) assertEquals(listOf("2", "4"), s) diff --git a/backend.native/tests/runtime/collections/hash_set0.kt b/backend.native/tests/runtime/collections/hash_set0.kt index 43d54e0b43b..5f9c95929ff 100644 --- a/backend.native/tests/runtime/collections/hash_set0.kt +++ b/backend.native/tests/runtime/collections/hash_set0.kt @@ -103,9 +103,9 @@ fun testContainsAll() { } fun testRemoveAll() { - val s = HashSet(listOf("1", "2", "3", "4", "5")) + val s = HashSet(listOf("1", "2", "3", "4", "5", "1")) assertFalse(s.removeAll(listOf("6", "7", "8"))) - assertEquals(setOf("1", "2", "3", "4", "5"), s) + assertEquals(setOf("1", "2", "3", "4", "5", "1"), s) assertTrue(s.removeAll(listOf("5", "3", "1"))) assertEquals(setOf("2", "4"), s) } diff --git a/runtime/src/main/kotlin/kotlin/collections/AbstractCollection.kt b/runtime/src/main/kotlin/kotlin/collections/AbstractCollection.kt index 451bdfa4e02..60e2392079c 100644 --- a/runtime/src/main/kotlin/kotlin/collections/AbstractCollection.kt +++ b/runtime/src/main/kotlin/kotlin/collections/AbstractCollection.kt @@ -49,3 +49,52 @@ public abstract class AbstractCollection protected constructor() : Collec */ protected open fun toArray(array: Array): Array = collectionToArray(this, array) } + +public abstract class AbstractMutableCollection protected constructor(): MutableCollection, AbstractCollection() { + + // Bulk Modification Operations + /** + * Adds all of the elements in the specified collection to this collection. + * + * @return `true` if any of the specified elements was added to the collection, `false` if the collection was not modified. + */ + override public fun addAll(elements: Collection): Boolean { + var changed = false + for (v in elements) { + if(add(v)) changed = true + } + return changed + } + + /** + * Removes all of this collection's elements that are also contained in the specified collection. + * + * @return `true` if any of the specified elements was removed from the collection, `false` if the collection was not modified. + */ + override public fun removeAll(elements: Collection): Boolean = removeAll(elements, true) + + /** + * Retains only the elements in this collection that are contained in the specified collection. + * + * @return `true` if any element was removed from the collection, `false` if the collection was not modified. + */ + override public fun retainAll(elements: Collection): Boolean = removeAll(elements, false) + + /** + * Removes the elements in this collection that are contained (if [contained] == true) or + * not contained (if [contained] == false) in [elements]. + * + * @return `true` if the collection has been modified and `false` otherwise. + */ + protected fun removeAll(elements: Collection, contained: Boolean): Boolean { + val it = iterator() + var changed = false + while (it.hasNext()) { + if (elements.contains(it.next()) == contained) { + it.remove() + changed = true + } + } + return changed + } +} diff --git a/runtime/src/main/kotlin/kotlin/collections/ArrayList.kt b/runtime/src/main/kotlin/kotlin/collections/ArrayList.kt index 541cfb88e7c..f972180eba7 100644 --- a/runtime/src/main/kotlin/kotlin/collections/ArrayList.kt +++ b/runtime/src/main/kotlin/kotlin/collections/ArrayList.kt @@ -21,7 +21,7 @@ class ArrayList private constructor( private var offset: Int, private var length: Int, private val backing: ArrayList? -) : MutableList, RandomAccess { +) : MutableList, RandomAccess, AbstractMutableCollection() { constructor() : this(10) @@ -131,19 +131,11 @@ class ArrayList private constructor( } override fun removeAll(elements: Collection): Boolean { - var changed = false - val it = iterator() - while (it.hasNext()) { - if (elements.contains(it.next())) { - it.remove() - changed = true - } - } - return changed + return retainOrRemoveAllInternal(offset, length, elements, false) > 0 } override fun retainAll(elements: Collection): Boolean { - return retainAllInRangeInternal(offset, length, elements) > 0 + return retainOrRemoveAllInternal(offset, length, elements, true) > 0 } override fun subList(fromIndex: Int, toIndex: Int): MutableList { @@ -274,16 +266,17 @@ class ArrayList private constructor( length -= rangeLength } - private fun retainAllInRangeInternal(rangeOffset: Int, rangeLength: Int, elements: Collection): Int { + /** Retains elements if [retain] == true and removes them it [retain] == false. */ + private fun retainOrRemoveAllInternal(rangeOffset: Int, rangeLength: Int, elements: Collection, retain: Boolean): Int { if (backing != null) { - val removed = backing.retainAllInRangeInternal(rangeOffset, rangeLength, elements) + val removed = backing.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain) length -= removed return removed } else { var i = 0 var j = 0 while (i < rangeLength) { - if (elements.contains(array[rangeOffset + i])) { + if (elements.contains(array[rangeOffset + i]) == retain) { array[rangeOffset + j++] = array[rangeOffset + i++] } else { i++ diff --git a/runtime/src/main/kotlin/kotlin/collections/HashSet.kt b/runtime/src/main/kotlin/kotlin/collections/HashSet.kt index 5bec9cde9e4..eaaa0ca782c 100644 --- a/runtime/src/main/kotlin/kotlin/collections/HashSet.kt +++ b/runtime/src/main/kotlin/kotlin/collections/HashSet.kt @@ -18,7 +18,7 @@ package kotlin.collections class HashSet internal constructor( val backing: HashMap -) : MutableSet { +) : MutableSet, AbstractMutableCollection() { constructor() : this(HashMap()) @@ -55,30 +55,6 @@ class HashSet internal constructor( return updated } - override fun removeAll(elements: Collection): Boolean { - val it = iterator() - var updated = false - while (it.hasNext()) { - if (elements.contains(it.next())) { - it.remove() - updated = true - } - } - return updated - } - - override fun retainAll(elements: Collection): Boolean { - val it = iterator() - var updated = false - while (it.hasNext()) { - if (!elements.contains(it.next())) { - it.remove() - updated = true - } - } - return updated - } - override fun equals(other: Any?): Boolean { return other === this || (other is Set<*>) &&