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

[alexismanin]

Related issue: KT-75801
Related benchmark project

This contains two different cases of optimization:

• Array<T>.(take|takeLast|takeWhile) functions : replace loop copy with copyOfRange().asList()
• Array<T>.toList() and Array<Array<T>>.flatten() : force using direct array copy instead of delegating to mutable list.
  This is done because at least OpenJDK implementation causes two array copies to happen instead of a single one.

I've not modified any primitive array function, because of unboxing. If I'd applied the same logic to primitive arrays, the initial copy might indeed get faster, but then, unboxing would be delayed on List.get() calls, therefore adding performance penalty on the consumer side.

I tested using ./gradlew coreLibsTest (success). I'm still trying to run a full ./gradlew build, but I've got configuration issues to investigate:

Task with path ':plugins:plugin-sandbox:plugin-annotations:jar' not found in project ':analysis:analysis-test-framework

It is my first contribution here, so if I've missed a point or done something wrong (formatting, doc, testing, etc.), any insight is appreciated.

[hoangchungk53qx1]

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()
	}

[alexismanin]

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 :

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

[alexismanin]

@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:

[hoangchungk53qx1]

Nice @alexismanin

[alexismanin]

I've pushed the change to takeWhile function.

[hoangchungk53qx1]

can early return

if (isEmpty()) return emptyList()

[hoangchungk53qx1]

My suggest

public fun <T> Array<out Array<out T>>.flatten(): List<T> {
    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
    }

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

[alexismanin]

Thanks for the suggestion. I've applied it.

[alexismanin]

@fzhinkin and @hoangchungk53qx1 : Is there anything else you need me to do or check before merging ?