From 7060811c8b25678e7046f9fb569a8f0f1f782100 Mon Sep 17 00:00:00 2001 From: Abduqodiri Qurbonzoda Date: Wed, 7 Sep 2022 02:01:09 +0300 Subject: [PATCH] Remove the brittle `contains` optimization code #KT-47707 --- .../common/src/generated/_Collections.kt | 19 +----- .../stdlib/common/src/generated/_Sequences.kt | 19 +----- .../stdlib/common/src/generated/_Sets.kt | 14 +--- libraries/stdlib/js/src/kotlin/collections.kt | 2 - .../src/kotlin/collections/CollectionsJVM.kt | 10 --- .../src/kotlin/collections/Collections.kt | 2 - .../BrittleContainsOptimization.kt | 66 ------------------- .../kotlin/collections/MutableCollections.kt | 27 ++++---- .../src/templates/Generators.kt | 32 ++------- 9 files changed, 30 insertions(+), 161 deletions(-) delete mode 100644 libraries/stdlib/src/kotlin/collections/BrittleContainsOptimization.kt diff --git a/libraries/stdlib/common/src/generated/_Collections.kt b/libraries/stdlib/common/src/generated/_Collections.kt index 149f9b4b304..debb11a6521 100644 --- a/libraries/stdlib/common/src/generated/_Collections.kt +++ b/libraries/stdlib/common/src/generated/_Collections.kt @@ -3141,26 +3141,17 @@ public operator fun Iterable.minus(element: T): List { /** * Returns a list containing all elements of the original collection except the elements contained in the given [elements] array. - * - * Before Kotlin 1.6, the [elements] array may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Iterable.minus(elements: Array): List { if (elements.isEmpty()) return this.toList() - val other = elements.convertToSetForSetOperation() - return this.filterNot { it in other } + return this.filterNot { it in elements } } /** * Returns a list containing all elements of the original collection except the elements contained in the given [elements] collection. - * - * Before Kotlin 1.6, the [elements] collection may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Iterable.minus(elements: Iterable): List { - val other = elements.convertToSetForSetOperationWith(this) + val other = elements.convertToListIfNotCollection() if (other.isEmpty()) return this.toList() return this.filterNot { it in other } @@ -3168,13 +3159,9 @@ public operator fun Iterable.minus(elements: Iterable): List { /** * Returns a list containing all elements of the original collection except the elements contained in the given [elements] sequence. - * - * Before Kotlin 1.6, the [elements] sequence may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Iterable.minus(elements: Sequence): List { - val other = elements.convertToSetForSetOperation() + val other = elements.toList() if (other.isEmpty()) return this.toList() return this.filterNot { it in other } diff --git a/libraries/stdlib/common/src/generated/_Sequences.kt b/libraries/stdlib/common/src/generated/_Sequences.kt index 10cfa809ba8..f3789d14db1 100644 --- a/libraries/stdlib/common/src/generated/_Sequences.kt +++ b/libraries/stdlib/common/src/generated/_Sequences.kt @@ -2610,10 +2610,6 @@ public operator fun Sequence.minus(element: T): Sequence { * * Note that the source sequence and the array being subtracted are iterated only when an `iterator` is requested from * the resulting sequence. Changing any of them between successive calls to `iterator` may affect the result. - * - * Before Kotlin 1.6, the [elements] array may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. * * The operation is _intermediate_ and _stateful_. */ @@ -2621,8 +2617,7 @@ public operator fun Sequence.minus(elements: Array): Sequence { if (elements.isEmpty()) return this return object: Sequence { override fun iterator(): Iterator { - val other = elements.convertToSetForSetOperation() - return this@minus.filterNot { it in other }.iterator() + return this@minus.filterNot { it in elements }.iterator() } } } @@ -2632,17 +2627,13 @@ public operator fun Sequence.minus(elements: Array): Sequence { * * Note that the source sequence and the collection being subtracted are iterated only when an `iterator` is requested from * the resulting sequence. Changing any of them between successive calls to `iterator` may affect the result. - * - * Before Kotlin 1.6, the [elements] collection may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. * * The operation is _intermediate_ and _stateful_. */ public operator fun Sequence.minus(elements: Iterable): Sequence { return object: Sequence { override fun iterator(): Iterator { - val other = elements.convertToSetForSetOperation() + val other = elements.convertToListIfNotCollection() if (other.isEmpty()) return this@minus.iterator() else @@ -2658,15 +2649,11 @@ public operator fun Sequence.minus(elements: Iterable): Sequence { * the resulting sequence. Changing any of them between successive calls to `iterator` may affect the result. * * The operation is _intermediate_ for this sequence and _terminal_ and _stateful_ for the [elements] sequence. - * - * Before Kotlin 1.6, the [elements] sequence may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Sequence.minus(elements: Sequence): Sequence { return object: Sequence { override fun iterator(): Iterator { - val other = elements.convertToSetForSetOperation() + val other = elements.toList() if (other.isEmpty()) return this@minus.iterator() else diff --git a/libraries/stdlib/common/src/generated/_Sets.kt b/libraries/stdlib/common/src/generated/_Sets.kt index 72e9df6c189..0283b9e6c34 100644 --- a/libraries/stdlib/common/src/generated/_Sets.kt +++ b/libraries/stdlib/common/src/generated/_Sets.kt @@ -32,10 +32,6 @@ public operator fun Set.minus(element: T): Set { * Returns a set containing all elements of the original set except the elements contained in the given [elements] array. * * The returned set preserves the element iteration order of the original set. - * - * Before Kotlin 1.6, the [elements] array may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Set.minus(elements: Array): Set { val result = LinkedHashSet(this) @@ -47,13 +43,9 @@ public operator fun Set.minus(elements: Array): Set { * Returns a set containing all elements of the original set except the elements contained in the given [elements] collection. * * The returned set preserves the element iteration order of the original set. - * - * Before Kotlin 1.6, the [elements] collection may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Set.minus(elements: Iterable): Set { - val other = elements.convertToSetForSetOperationWith(this) + val other = elements.convertToListIfNotCollection() if (other.isEmpty()) return this.toSet() if (other is Set) @@ -67,10 +59,6 @@ public operator fun Set.minus(elements: Iterable): Set { * Returns a set containing all elements of the original set except the elements contained in the given [elements] sequence. * * The returned set preserves the element iteration order of the original set. - * - * Before Kotlin 1.6, the [elements] sequence may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - * a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - * On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. */ public operator fun Set.minus(elements: Sequence): Set { val result = LinkedHashSet(this) diff --git a/libraries/stdlib/js/src/kotlin/collections.kt b/libraries/stdlib/js/src/kotlin/collections.kt index 406979519c5..0c0f0376485 100644 --- a/libraries/stdlib/js/src/kotlin/collections.kt +++ b/libraries/stdlib/js/src/kotlin/collections.kt @@ -260,5 +260,3 @@ internal actual fun mapCapacity(expectedSize: Int) = expectedSize internal fun checkBuilderCapacity(capacity: Int) { require(capacity >= 0) { "capacity must be non-negative." } } - -internal actual fun brittleContainsOptimizationEnabled(): Boolean = false \ No newline at end of file diff --git a/libraries/stdlib/jvm/src/kotlin/collections/CollectionsJVM.kt b/libraries/stdlib/jvm/src/kotlin/collections/CollectionsJVM.kt index df5b0dd4173..04fff9dc6b0 100644 --- a/libraries/stdlib/jvm/src/kotlin/collections/CollectionsJVM.kt +++ b/libraries/stdlib/jvm/src/kotlin/collections/CollectionsJVM.kt @@ -117,13 +117,3 @@ internal actual inline fun checkCountOverflow(count: Int): Int { } return count } - - -@Suppress("NOTHING_TO_INLINE") -internal actual inline fun brittleContainsOptimizationEnabled(): Boolean = CollectionSystemProperties.brittleContainsOptimizationEnabled - -internal object CollectionSystemProperties { - @JvmField - internal val brittleContainsOptimizationEnabled: Boolean = - System.getProperty("kotlin.collections.convert_arg_to_set_in_removeAll")?.toBoolean() ?: false -} \ No newline at end of file diff --git a/libraries/stdlib/native-wasm/src/kotlin/collections/Collections.kt b/libraries/stdlib/native-wasm/src/kotlin/collections/Collections.kt index ce8b1cd69ab..520dc1040e6 100644 --- a/libraries/stdlib/native-wasm/src/kotlin/collections/Collections.kt +++ b/libraries/stdlib/native-wasm/src/kotlin/collections/Collections.kt @@ -133,5 +133,3 @@ internal actual inline fun checkCountOverflow(count: Int): Int { } return count } - -internal actual fun brittleContainsOptimizationEnabled(): Boolean = false diff --git a/libraries/stdlib/src/kotlin/collections/BrittleContainsOptimization.kt b/libraries/stdlib/src/kotlin/collections/BrittleContainsOptimization.kt deleted file mode 100644 index 12af4aedcdf..00000000000 --- a/libraries/stdlib/src/kotlin/collections/BrittleContainsOptimization.kt +++ /dev/null @@ -1,66 +0,0 @@ -/* - * Copyright 2010-2021 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 kotlin.collections - - -/** Returns true if the brittle contains optimization is enabled. See KT-45438. */ -internal expect fun brittleContainsOptimizationEnabled(): Boolean - -/** - * Returns true if [brittleContainsOptimizationEnabled] is true - * and it's safe to convert this collection to a set without changing contains method behavior. - */ -private fun Collection.safeToConvertToSet() = brittleContainsOptimizationEnabled() && size > 2 && this is ArrayList - -/** - * When [brittleContainsOptimizationEnabled] is true: - * - Converts this [Iterable] to a set if it is not a [Collection]. - * - Converts this [Collection] to a set, when it's worth so and it doesn't change contains method behavior. - * - Otherwise returns this. - * When [brittleContainsOptimizationEnabled] is false: - * - Converts this [Iterable] to a list if it is not a [Collection]. - * - Otherwise returns this. - */ -internal fun Iterable.convertToSetForSetOperationWith(source: Iterable): Collection = - when (this) { - is Set -> this - is Collection -> - when { - source is Collection && source.size < 2 -> this - else -> if (this.safeToConvertToSet()) toHashSet() else this - } - else -> if (brittleContainsOptimizationEnabled()) toHashSet() else toList() - } - -/** - * When [brittleContainsOptimizationEnabled] is true: - * - Converts this [Iterable] to a set if it is not a [Collection]. - * - Converts this [Collection] to a set, when it's worth so and it doesn't change contains method behavior. - * - Otherwise returns this. - * When [brittleContainsOptimizationEnabled] is false: - * - Converts this [Iterable] to a list if it is not a [Collection]. - * - Otherwise returns this. - */ -internal fun Iterable.convertToSetForSetOperation(): Collection = - when (this) { - is Set -> this - is Collection -> if (this.safeToConvertToSet()) toHashSet() else this - else -> if (brittleContainsOptimizationEnabled()) toHashSet() else toList() - } - -/** - * Converts this sequence to a set if [brittleContainsOptimizationEnabled] is true, - * otherwise converts it to a list. - */ -internal fun Sequence.convertToSetForSetOperation(): Collection = - if (brittleContainsOptimizationEnabled()) toHashSet() else toList() - -/** - * Converts this array to a set if [brittleContainsOptimizationEnabled] is true, - * otherwise converts it to a list. - */ -internal fun Array.convertToSetForSetOperation(): Collection = - if (brittleContainsOptimizationEnabled()) toHashSet() else asList() \ No newline at end of file diff --git a/libraries/stdlib/src/kotlin/collections/MutableCollections.kt b/libraries/stdlib/src/kotlin/collections/MutableCollections.kt index 6dbda52670a..c1219130a15 100644 --- a/libraries/stdlib/src/kotlin/collections/MutableCollections.kt +++ b/libraries/stdlib/src/kotlin/collections/MutableCollections.kt @@ -8,8 +8,6 @@ package kotlin.collections -import kotlin.random.Random - /** * Removes a single instance of the specified element from this * collection, if it is present. @@ -141,33 +139,40 @@ public fun MutableCollection.addAll(elements: Array): Boolean { return addAll(elements.asList()) } +/** + * Converts this [Iterable] to a list if it is not a [Collection]. + * Otherwise, returns this. + */ +internal fun Iterable.convertToListIfNotCollection(): Collection = + if (this is Collection) this else toList() + /** * Removes all elements from this [MutableCollection] that are also contained in the given [elements] collection. */ public fun MutableCollection.removeAll(elements: Iterable): Boolean { - return removeAll(elements.convertToSetForSetOperationWith(this)) + return removeAll(elements.convertToListIfNotCollection()) } /** * Removes all elements from this [MutableCollection] that are also contained in the given [elements] sequence. */ public fun MutableCollection.removeAll(elements: Sequence): Boolean { - val set = elements.convertToSetForSetOperation() - return set.isNotEmpty() && removeAll(set) + val list = elements.toList() + return list.isNotEmpty() && removeAll(list) } /** * Removes all elements from this [MutableCollection] that are also contained in the given [elements] array. */ public fun MutableCollection.removeAll(elements: Array): Boolean { - return elements.isNotEmpty() && removeAll(elements.convertToSetForSetOperation()) + return elements.isNotEmpty() && removeAll(elements.asList()) } /** * Retains only elements of this [MutableCollection] that are contained in the given [elements] collection. */ public fun MutableCollection.retainAll(elements: Iterable): Boolean { - return retainAll(elements.convertToSetForSetOperationWith(this)) + return retainAll(elements.convertToListIfNotCollection()) } /** @@ -175,7 +180,7 @@ public fun MutableCollection.retainAll(elements: Iterable): Boolean */ public fun MutableCollection.retainAll(elements: Array): Boolean { if (elements.isNotEmpty()) - return retainAll(elements.convertToSetForSetOperation()) + return retainAll(elements.asList()) else return retainNothing() } @@ -184,9 +189,9 @@ public fun MutableCollection.retainAll(elements: Array): Boolea * Retains only elements of this [MutableCollection] that are contained in the given [elements] sequence. */ public fun MutableCollection.retainAll(elements: Sequence): Boolean { - val set = elements.convertToSetForSetOperation() - if (set.isNotEmpty()) - return retainAll(set) + val list = elements.toList() + if (list.isNotEmpty()) + return retainAll(list) else return retainNothing() } diff --git a/libraries/tools/kotlin-stdlib-gen/src/templates/Generators.kt b/libraries/tools/kotlin-stdlib-gen/src/templates/Generators.kt index 8c5662c4d57..183019b9cbc 100644 --- a/libraries/tools/kotlin-stdlib-gen/src/templates/Generators.kt +++ b/libraries/tools/kotlin-stdlib-gen/src/templates/Generators.kt @@ -367,13 +367,6 @@ object Generators : TemplateGroupBase() { } } - private fun elementsConversionClause(elements: Family) = - """ - Before Kotlin 1.6, the [elements] ${elements.doc.collection} may have been converted to a [HashSet] to speed up the operation, thus the elements were required to have - a correct and stable implementation of `hashCode()` that didn't change between successive invocations. - On JVM, you can enable this behavior back with the system property `kotlin.collections.convert_arg_to_set_in_removeAll` set to `true`. - """ - val f_minus_iterable = fn("minus(elements: Iterable)") { include(Iterables, Sets, Sequences) } builder { @@ -384,7 +377,7 @@ object Generators : TemplateGroupBase() { specialFor(Sets, Sequences) { returns("SELF") } body { """ - val other = elements.convertToSetForSetOperationWith(this) + val other = elements.convertToListIfNotCollection() if (other.isEmpty()) return this.toList() @@ -402,7 +395,7 @@ object Generators : TemplateGroupBase() { } body { """ - val other = elements.convertToSetForSetOperationWith(this) + val other = elements.convertToListIfNotCollection() if (other.isEmpty()) return this.toSet() if (other is Set) @@ -429,7 +422,7 @@ object Generators : TemplateGroupBase() { """ return object: Sequence { override fun iterator(): Iterator { - val other = elements.convertToSetForSetOperation() + val other = elements.convertToListIfNotCollection() if (other.isEmpty()) return this@minus.iterator() else @@ -440,9 +433,6 @@ object Generators : TemplateGroupBase() { } } - doc { - doc + elementsConversionClause(Iterables) - } } val f_minus_array = fn("minus(elements: Array)") { @@ -456,8 +446,7 @@ object Generators : TemplateGroupBase() { body { """ if (elements.isEmpty()) return this.toList() - val other = elements.convertToSetForSetOperation() - return this.filterNot { it in other } + return this.filterNot { it in elements } """ } specialFor(Sets) { @@ -492,16 +481,12 @@ object Generators : TemplateGroupBase() { if (elements.isEmpty()) return this return object: Sequence { override fun iterator(): Iterator { - val other = elements.convertToSetForSetOperation() - return this@minus.filterNot { it in other }.iterator() + return this@minus.filterNot { it in elements }.iterator() } } """ } } - doc { - doc + elementsConversionClause(ArraysOfObjects) - } } val f_minus_sequence = fn("minus(elements: Sequence)") { @@ -514,7 +499,7 @@ object Generators : TemplateGroupBase() { specialFor(Sets, Sequences) { returns("SELF") } body { """ - val other = elements.convertToSetForSetOperation() + val other = elements.toList() if (other.isEmpty()) return this.toList() @@ -553,7 +538,7 @@ object Generators : TemplateGroupBase() { """ return object: Sequence { override fun iterator(): Iterator { - val other = elements.convertToSetForSetOperation() + val other = elements.toList() if (other.isEmpty()) return this@minus.iterator() else @@ -563,9 +548,6 @@ object Generators : TemplateGroupBase() { """ } } - doc { - doc + elementsConversionClause(Sequences) - } } val f_partition = fn("partition(predicate: (T) -> Boolean)") {