Avoid creating new PersistentList instance when adding empty collection

[popematt]

Problem

I encountered unexpected growth in memory usage while working on an library that uses PersistentList. I eventually realized that it is because I had many distinct instances of an empty PersistentList.

I traced down the problem, starting with toPersistentList(), which I was using to convert all inputs before creating assembling it into my data model. The toPersistentList() function takes the empty persistent list and adds all elements of this to the empty list. When you add elements to a SmallPersistentVector, it checks whether the combined size is less than or equal to MAX_BUFFER_SIZE and if it is, it unconditionally creates a new SmallPersistentVector instance.

kotlinx.collections.immutable/core/commonMain/src/implementations/immutableList/SmallPersistentVector.kt

Lines 38 to 49 in 7fb0d74

	override fun addAll(elements: Collection<E>): PersistentList<E> {
	if (size + elements.size <= MAX_BUFFER_SIZE) {
	val newBuffer = buffer.copyOf(size + elements.size)
	// TODO: investigate performance of elements.toArray + copyInto
	var index = size
	for (element in elements) {
	newBuffer[index++] = element
	}
	return SmallPersistentVector(newBuffer)
	}
	return mutate { it.addAll(elements) }
	}

This means that no-ops on small lists (such as adding two empty lists) always results in a new instance being created.

I verified that the problem only exists for SmallPersistentVector. PersistentVector uses PersistentVectorBuilder, which already has checks for adding an empty collection.

What I am changing

I have added a check to fun addAll(elements: Collection<E>): PersistentList<E> and fun addAll(index: Int, c: Collection<E>): PersistentList<E> to return early if the collection of elements to be added is empty.

I have updated the tests to include missing no-op cases related to adding and removing empty collections in both ImmutableListTest and ImmutableSetTest, and I have added tests for toPersistentMap(), toPersistentSet(), and toPersistentList() to ensure that when the receiver is an empty map/collection, they always return the same instance of persistent map/set/list.

[JakeWharton]

There is assertSame for this, which should give a slightly better error message should it ever fail.

[popematt]

Yeah, my preference is to use assertSame() but I noticed that this particular file was using === instead of assertSame(), and I try not to introduce new things for small changes like this. I'm happy to change it, though.

[JakeWharton]

Makes sense. It probably pre-dates instance equality being added to kotlin.test! No need to change, was just making sure it was known.

[JakeWharton]

I'm not a maintainer, so best I can do is 👍

[qurbonzoda]

Thank you very much for the contribution!