Use same size for initial capacity of Builder in transformations

[Nillerr]

Hi @daniel-rusu!

Great idea! While most Kotlin applications certainly don't need to worry about squeezing out the efficiency of using something like ImmutableArray, it is undoubtedly a valuable tool to add to the Kotlin ecosystem. Your implementation is great!

This PR changes the use of Builder() to Builder(size) based on the idea that some of these operations would typically result in arrays of around the same size as the original array, while other operations will never result in an array exceeding the size of the original array, and as such having an initial capacity of the same size means we can avoid repeatedly resizing the array of the builder. E.g. for something like filter and mapNotNull we know that the result is at most an array of the same size as the original array, so by having an initial capacity capable of holding all the elements, we will see at most 2 array allocations.

• Have you followed the contributing guidelines?
• Have you added tests that validate the new capability or bug fix?
• Does your submission align with the current code style?
• Have you checked that there aren't other open pull requests for the same change?

Looking forward to your feedback!

[daniel-rusu]

Hi @daniel-rusu!

Great idea! While most Kotlin applications certainly don't need to worry about squeezing out the efficiency of using something like ImmutableArray, it is undoubtedly a valuable tool to add to the Kotlin ecosystem. Your implementation is great!

This PR changes the use of Builder() to Builder(size) based on the idea that some of these operations would typically result in arrays of around the same size as the original array, while other operations will never result in an array exceeding the size of the original array, and as such having an initial capacity of the same size means we can avoid repeatedly resizing the array of the builder. E.g. for something like filter and mapNotNull we know that the result is at most an array of the same size as the original array, so by having an initial capacity capable of holding all the elements, we will see at most 2 array allocations.

Hi @Nillerr , thanks so much for your suggestion! I thought a lot about whether to use some sort of initial capacity estimate for the builders when the resulting size isn't known in advance. There are some gotchas that would make the performance worse while also increasing memory consumption:

1. For operations like flatMap, we generally have something like a list of orders with each order having a list of products, and we want to flatten it into a large list of all products from all orders. Note that if you have less than 10 orders with each order having a bunch of products, setting the initial builder capacity to the number of orders will increase the number of resizing operations.

2. For operations like groupBy, we generally have something like a list of people that we want to group based on each person's favorite sport. Using the size of the initial array as the default capacity for each group will multiply the memory consumption from N elements to N X G elements where G is the number of groups. Note that G can be as high as N depending on the criteria for group determination so the memory consumption could jump to N^2 during this operation.

3. When filtering a very large array (filter / filterNotNull / mapNotNull / etc.), it's highly unlikely for all elements to pass the condition and it's also possible for only a tiny percentage to pass. Creating a builder with the original very large capacity will make it more difficult for the memory subsystem to locate sufficient contiguous memory resulting in more checks as it looks through the list of free memory chunks. Additionally, these large memory allocations are typically allocated directly in the tenured memory zone due to the weak-generational hypothesis which assumes that large objects will stick around a long time. The tenured space is typically only inspected during major garbage collections which last hundreds of times longer than minor collections. So adding memory directly into the tenured space will increase the rate of major GCs and negatively affect application performance potentially quite significantly.

So overall, I think that most developers would be disappointed if memory consumption increased after swapping read-only lists with Immutable Arrays. My goal is that Immutable Arrays would have the same or lower memory consumption while also improving performance.

Having said all that, I think we can approach this from a different angle and perhaps achieve the seemingly impossible result of getting the best of both worlds. Stepping back, the real reason we're trying to avoid re-sizing operations is because each resize has to copy all the current elements into the new larger array. I'm tinkering with builder enhancements that give us the best of both worlds by starting with low capacity and also avoiding any copying when resizing. The idea is for the builder to store a linked list of increasing arrays so that any new capacity would just create another link and leave the current elements as is. Copying from the links into the final array shouldn't be worse as this allows us to copy only a single time instead of each time it's resized. This should also minimize large memory requests as the builder will have the elements split into chunks instead of one much larger array. I haven't submitted any code yet as I'm tinkering with throw-away benchmarks to measure different ideas.

Just a heads up that my main focus right now is to improve documentation, add a bunch of missing features that are expected to be commonly used, and after that take a deeper look at improving performance further a few releases down the line.

[teenriot]

The filter method with initial size set allows to remove the builder with its capacity checks completely. That gave around 25% performance gain on my system for the test scenario you use for reference arrays. That's significant. May it is an option to define a size value for what is considered as "small" array and then use different implementations for "small" and "big" arrays.

Thats the code i benchmarked:

    @Suppress("UNCHECKED_CAST")
    public inline fun filter(predicate: (element: T) -> Boolean): ImmutableArray<T> {
        val buffer = arrayOfNulls<Any?>(size) as Array<T>
        var bufferSize = 0

        for (element in values) {
            if (predicate(element)) {
                buffer[bufferSize++] = element
            }
        }

        return when (bufferSize) {
            0 -> EMPTY
            size -> this
            else -> ImmutableArray<T>(buffer.copyOf(bufferSize) as Array<T>)
        }
    }

[teenriot]

I also tested another 'branchless' version that could be done when there is no builder. The results speak for itself.

Thats the code with branchless loop and without builder:

@Suppress("UNCHECKED_CAST")
public inline fun filter(predicate: (element: T) -> Boolean): ImmutableArray<T> {
    val buffer = arrayOfNulls<Any?>(size) as Array<T>
    var bufferSize = 0

    for (element in values) {
        buffer[bufferSize] = element
        bufferSize += predicate(element).asInt
    }

    return when (bufferSize) {
        0 -> EMPTY
        size -> this
        else -> ImmutableArray<T>(buffer.copyOf(bufferSize) as Array<T>)
    }
}
    
 /** @returns 1 if true otherwise 0 */
public inline val Boolean.asInt : Int get() = 1 and (hashCode() shr 1)

(pls let me know if this is the wrong place for my comments)

[teenriot]

Regarding:
The idea is for the builder to store a linked list of increasing arrays so that any new capacity would just create another link and leave the current elements as is

Thats some kind of an Unrolled linked list, i talked about here.