Detekt: Verify if the spread operator still has performance impacts

[arturbosch]

Like mentioned in #546 an issue with the spread operator seems to be fixed KT-20462.

We must rerun the benchmark to verify if the SpreadOperator rule is obsolete.

[3flex]

I've run the benchmark, and saw no change with 1.1.60. Looking over it again I realise that KT-20462 only addresses the case when an array constructor function is used with the spread operator, while the linked benchmark passes an array to a method, where they didn't claim any performance improvement.

So I think this means:

fun foo(vararg xs: Int) {}
foo(xs = *intArrayOf(1))

should no longer suffer array copy performance penalty, but I expect this still does:

fun foo(vararg xs: Int) {}
val numbers = intArrayOf(10, 20, 30, 40, 50)
foo(xs = *numbers)

The check that was added to the compiler to check for cases where array copy can be skipped is:

    private boolean canSkipArrayCopyForSpreadArgument(KtExpression spreadArgument) {
        ResolvedCall<? extends CallableDescriptor> resolvedCall = CallUtilKt.getResolvedCall(spreadArgument, bindingContext);
        if (resolvedCall == null) return false;

        CallableDescriptor calleeDescriptor = resolvedCall.getResultingDescriptor();
        return (calleeDescriptor instanceof ConstructorDescriptor) ||
               CompileTimeConstantUtils.isArrayFunctionCall(resolvedCall) ||
               (DescriptorUtils.getFqName(calleeDescriptor).asString().equals("kotlin.arrayOfNulls"));
    }

That check could be added to SpreadOperator check, and only flag those where above method returns false.

Edit: Note that implementing that change to SpreadOperator requires type and symbol resolving.

[rock3r]

Is this still an issue on 1.3.x?

[3flex]

The canSkipArrayCopyForSpreadArgument method is still used within the Kotlin compiler as of 1.3.21 so I would suggest there are still cases where the array copy cannot be skipped, so this remains an issue.

https://github.com/JetBrains/kotlin/blob/v1.3.21/compiler/backend/src/org/jetbrains/kotlin/codegen/ExpressionCodegen.java#L2913-L2921

[3flex]

I've reconfirmed in 1.3.30, checking the Kotlin bytecode in IntelliJ. When the spread operator is used there's a call of INVOKESTATIC java/util/Arrays.copyOf when the spread operator is used, except when an array constructor is used as an argument for the vararg parameter.

fun doStuff(vararg data: String) {}

fun doStuff2(vararg data: String, moreData: Int) {}

fun doStuff3() {
    // no copy for these cases
    doStuff(*Array(1) {"a"})
    doStuff2(data = *arrayOf("a", "b", "c"), moreData = 3)

    // array copied in these cases
    val arr = arrayOf("a", "b", "c")
    doStuff(*arr)
    doStuff2(data = *arr, moreData = 3)
}

With type resolution this can be checked based on the method mentioned in my last comment so the case where an array constructor is used will effectively be whitelisted.