Skip to content

[KT-75801]: minor optimization on object array to list conversion functions #5426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[KT-75801]: minor optimization on object array to list conversion functions #5426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f27385
KT-75801: Optimize stdlib Array<Array<T>.flatten() function
alexismanin Mar 28, 2025
b27f970
KT-75801: Optimize stdlib take, takeLast and takeWhile functions
alexismanin Apr 3, 2025
a27bac7
KT-75801: Optimize stdlib Array<T>.toList() function
alexismanin Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 8 additions & 20 deletions libraries/stdlib/common/src/generated/_Arrays.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4806,14 +4806,7 @@ public fun <T> Array<out T>.take(n: Int): List<T> {
if (n == 0) return emptyList()
if (n >= size) return toList()
if (n == 1) return listOf(this[0])
var count = 0
val list = ArrayList<T>(n)
for (item in this) {
list.add(item)
if (++count == n)
break
}
return list
return copyOfRange(0, n).asList()
}

/**
Expand Down Expand Up @@ -5005,10 +4998,7 @@ public fun <T> Array<out T>.takeLast(n: Int): List<T> {
val size = size
if (n >= size) return toList()
if (n == 1) return listOf(this[size - 1])
val list = ArrayList<T>(n)
for (index in size - n until size)
list.add(this[index])
return list
return copyOfRange(size - n, size).asList()
}

/**
Expand Down Expand Up @@ -5295,13 +5285,11 @@ public inline fun CharArray.takeLastWhile(predicate: (Char) -> Boolean): List<Ch
* @sample samples.collections.Collections.Transformations.take
*/
public inline fun <T> Array<out T>.takeWhile(predicate: (T) -> Boolean): List<T> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will be easier to read, i think, with equivalent performance

Suggested change
public inline fun <T> Array<out T>.takeWhile(predicate: (T) -> Boolean): List<T> {
public inline fun <T> Array<out T>.takeWhile(predicate: (T) -> Boolean): List<T> {
var i = 0
while (i < size && predicate(this[i])) i++
return if (i == 0) emptyList() else this.copyOfRange(0, i).asList()
}

Copy link
Author

@alexismanin alexismanin Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the impact of removing the special case handling, and it looks like it is noticeable. On the benchmark result below, your suggestion is the "takeWhileSuggestion" case. We see that for array size 0 and 1, it is a little less fast than the proposed optimization :

image

Therefore, I think we should keep the when as it is now.

P.S: If you want to double-check this, I've added the suggestions in my benchmark project on the test-pr-suggestions branch

Copy link
Author

@alexismanin alexismanin Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoangchungk53qx1 : After second thought, your simplification makes sense.

I realized that it just miss a condition to improve its performance when output list has only one element (which is a broader case than my current implementation, which only accounts for cases when input array has only one element) :

return if (i == 0) emptyList()
       else if (i == 1) listOf(this[0])
       else copyOfRange(0, i).asList()

I've launched a benchmark by modifying your suggestion so that the final return condition accounts for cases where i is 1. It greatly improves performance, for two cases of the benchmark :

  1. Input array has one element
  2. Output list has only one element (in the benchmark, this is the case where input array is of size 3. We only take the first element from it)

I will soon apply you suggestion with this tweak, and I think we will be good then.

Here is a glimpse of updated benchmark results:

image

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @alexismanin

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed the change to takeWhile function.

val list = ArrayList<T>()
for (item in this) {
if (!predicate(item))
break
list.add(item)
}
return list
var i = 0
while (i < size && predicate(this[i])) i++
return if (i == 0) emptyList()
else if (i == 1) listOf(this[0])
else copyOfRange(0, i).asList()
}

/**
Expand Down Expand Up @@ -9838,7 +9826,7 @@ public fun <T> Array<out T>.toList(): List<T> {
return when (size) {
0 -> emptyList()
1 -> listOf(this[0])
else -> this.toMutableList()
else -> copyOf().asList()
}
}

Expand Down
20 changes: 16 additions & 4 deletions libraries/stdlib/src/kotlin/collections/Arrays.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,23 @@ import kotlin.contracts.*
* @sample samples.collections.Arrays.Transformations.flattenArray
*/
public fun <T> Array<out Array<out T>>.flatten(): List<T> {
val result = ArrayList<T>(sumOf { it.size })
for (element in this) {
result.addAll(element)
if (isEmpty()) return emptyList()

val totalSizeLong = sumOf { it.size.toLong() }
if (totalSizeLong == 0L) return emptyList()
require(totalSizeLong <= Int.MAX_VALUE.toLong()) {
"Sum of all arrays overflow maximum array capacity (of Int.MAX_VALUE)"
}

val outputArray = arrayOfNulls<Any?>(totalSizeLong.toInt())
var offset = 0
for (innerArray in this) {
innerArray.copyInto(outputArray, offset)
offset += innerArray.size
}
return result

@Suppress("UNCHECKED_CAST")
return outputArray.asList() as List<T>
}

/**
Expand Down
39 changes: 37 additions & 2 deletions libraries/tools/kotlin-stdlib-gen/src/templates/Filtering.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ object Filtering : TemplateGroupBase() {
"""
}

body(ArraysOfObjects, ArraysOfPrimitives, ArraysOfUnsigned) {
body(ArraysOfPrimitives, ArraysOfUnsigned) {
"""
require(n >= 0) { "Requested element count $n is less than zero." }
if (n == 0) return emptyList()
Expand All @@ -208,6 +208,17 @@ object Filtering : TemplateGroupBase() {
return list
"""
}

// For object arrays, ensure a single array copy instead of copying using a loop (see KT-75801)
body(ArraysOfObjects) {
"""
require(n >= 0) { "Requested element count $n is less than zero." }
if (n == 0) return emptyList()
if (n >= size) return toList()
if (n == 1) return listOf(this[0])
return copyOfRange(0, n).asList()
"""
}
}

val f_dropLast = fn("dropLast(n: Int)") {
Expand Down Expand Up @@ -270,7 +281,7 @@ object Filtering : TemplateGroupBase() {
"""
}

body(ArraysOfObjects, ArraysOfPrimitives, ArraysOfUnsigned) {
body(ArraysOfPrimitives, ArraysOfUnsigned) {
"""
require(n >= 0) { "Requested element count $n is less than zero." }
if (n == 0) return emptyList()
Expand All @@ -284,6 +295,19 @@ object Filtering : TemplateGroupBase() {
return list
"""
}

// For object arrays, ensure a single array copy instead of copying using a loop (see KT-75801)
body(ArraysOfObjects) {
"""
require(n >= 0) { "Requested element count $n is less than zero." }
if (n == 0) return emptyList()
val size = size
if (n >= size) return toList()
if (n == 1) return listOf(this[size - 1])
return copyOfRange(size - n, size).asList()
"""
}

body(Lists) {
"""
require(n >= 0) { "Requested element count $n is less than zero." }
Expand Down Expand Up @@ -413,6 +437,17 @@ object Filtering : TemplateGroupBase() {
returns("Sequence<T>")
body { """return TakeWhileSequence(this, predicate)""" }
}

// For object arrays, ensure a single array copy instead of copying using a loop (see KT-75801)
body(ArraysOfObjects) {
"""
var i = 0
while (i < ${f.code.size} && predicate(this[i])) i++
return if (i == 0) emptyList()
else if (i == 1) listOf(this[0])
else copyOfRange(0, i).asList()
"""
}
}

val f_dropLastWhile = fn("dropLastWhile(predicate: (T) -> Boolean)") {
Expand Down
12 changes: 11 additions & 1 deletion libraries/tools/kotlin-stdlib-gen/src/templates/Snapshots.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ object Snapshots : TemplateGroupBase() {
return this.toMutableList().optimizeReadOnlyList()
"""
}
body(CharSequences, ArraysOfPrimitives, ArraysOfObjects) {
body(CharSequences, ArraysOfPrimitives) {
"""
return when (${f.code.size}) {
0 -> emptyList()
Expand All @@ -182,6 +182,16 @@ object Snapshots : TemplateGroupBase() {
}
"""
}
// For object array, ensure a single array copy instead of delegating to `toMutableList`, which can cause two copies (see KT-75801)
body(ArraysOfObjects) {
"""
return when (${f.code.size}) {
0 -> emptyList()
1 -> listOf(this[0])
else -> copyOf().asList()
}
"""
}
body(Sequences) { optimizedSequenceToCollection("emptyList", "listOf", "ArrayList") }
specialFor(Maps) {
doc { "Returns a [List] containing all key-value pairs." }
Expand Down