-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Optimize sorted()
for primitive-type arrays
#5419
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.
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
base: master
Are you sure you want to change the base?
Conversation
@pubiqq, nice catch! Thanks for opening the PR. I slightly modified your benchmark to both validate results and check how the updated version of For regular primitive array types the change improves For unsigned array types, however, the change does not affect performance, so it's better to abstain from updating corresponding implementations (or try to find a way to speed them up too). Also, it is worth updating |
The changes to the unsigned arrays were added only to make the However, okay, I'll roll it back.
|
^KT-76423 Fixed
4a88cb0
to
8311d96
Compare
@@ -6283,49 +6283,49 @@ public fun <T : Comparable<T>> Array<out T>.sorted(): List<T> { | |||
* Returns a list of all elements sorted according to their natural sort order. | |||
*/ | |||
public fun ByteArray.sorted(): List<Byte> { | |||
return toTypedArray().apply { sort() }.asList() | |||
return sortedArray().asList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, asList()
function was invoked on a Array<T>
, now it is invoked on a corresponding primitive array.
The change is subtle, but it results in different underlying types of the resulting list. Further analysis is required to determine how this change may affect users. For example, some codebases may rely on the fact that the resulting list is effectively mutable and explicitly cast it to MutableList<T>
. Changed underlying type may also affect call sites where a list produced by the sorted()
call is used. This need to be investigated further.
Besides these potential issues, there is a less ephemeral one: PrimitiveArray.asList()
returns a wrapper around the primitive array (for example,
public actual fun IntArray.asList(): List<Int> { |
As a middle ground, sorted
may convert sorted primitive array into a typed array first and then invoke asList
on it (sortedArray().toTypedArray().asList()
). Such an implementation outperforms the existing one (by saving time on sorting itself), but returns exactly the same list type as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides these potential issues, there is a less ephemeral one:
PrimitiveArray.asList()
returns a wrapper around the primitive array ... As a result, access to individual elements requires boxing, which results in worse performance in general.
The benchmark results (https://github.com/pubiqq/benchmarks-kmp/tree/list-get-benchmarks) do not allow us to state this unequivocally, in most cases accessing a primitive from PrimitiveArray.asList()
is faster than from PrimitiveArray.toTypedArray().asList()
.
It should also be noted that toTypedArray()
boxes primitives too. Yes, it does this preemptively, but it does this for all elements, and this is not always better than using a list that converts primitives on demand.
As a middle ground,
sorted
may convert sorted primitive array into a typed array first and then invoke asList on it (sortedArray().toTypedArray().asList()
). Such an implementation outperforms the existing one (by saving time on sorting itself), but returns exactly the same list type as before.
Such an implementation also performs two array copy operations instead of one. I'm not sure it's worth it.
See KT-76423