Some new performance features #14
Conversation
ericeil
changed the title
| private fun <R : Any> mapReduceImpl(map: (K, V) -> R, reduce: (R, R) -> R): R { | ||
| val (left, middle, right) = fork( | ||
| self, | ||
| { left?.mapReduceImpl(map, reduce) }, | ||
| { shallowMapReduce(map, reduce) }, | ||
| { right?.mapReduceImpl(map, reduce) } | ||
| ) | ||
| val leftAndMiddle = left?.let { reduce(it, middle) } ?: middle | ||
| return right?.let { reduce(leftAndMiddle, it) } ?: leftAndMiddle | ||
| } |
There was a problem hiding this comment.
is there a compelling reason to not just go ahead and make this an erased types implementation too? It seems like we always go back and do this for performance anyway so...
There was a problem hiding this comment.
Static typing is good. :)
| override fun <R : Any> mapReduce(map: (E) -> R, reduce: (R, R) -> R): R = | ||
| notForking(self) { mapReduceImpl(map, reduce) } | ||
|
|
||
| override fun <R : Any> parallelMapReduce(map: (E) -> R, reduce: (R, R) -> R, parallelThresholdLog2: Int): R = | ||
| maybeForking(self, threshold = { it.isApproximatelySmallerThanLog2(parallelThresholdLog2) }) { | ||
| mapReduceImpl(map, reduce) | ||
| } | ||
|
|
||
| context(ThresholdForker<S>) | ||
| private fun <R : Any> mapReduceImpl(map: (E) -> R, reduce: (R, R) -> R): R { | ||
| val (left, middle, right) = fork( | ||
| self, | ||
| { left?.mapReduceImpl(map, reduce) }, | ||
| { shallowMapReduce(map, reduce) }, | ||
| { right?.mapReduceImpl(map, reduce) } | ||
| ) | ||
| val leftAndMiddle = left?.let { reduce(it, middle) } ?: middle | ||
| return right?.let { reduce(leftAndMiddle, it) } ?: leftAndMiddle | ||
| } |
There was a problem hiding this comment.
just so I'm not crazy: this is the same implementation as the map case right?
There was a problem hiding this comment.
Yes, I think it's just the map lambda type that's different.
| override fun <R : Any> updateValues( | ||
| transform: (K, V) -> R? | ||
| ): TreapMap<K, R> = treapMapOf() | ||
|
|
||
| override fun <U> updateEntry(key: K, value: U?, merger: (V?, U?) -> V?): TreapMap<K, V> = | ||
| override fun <R : Any> parallelUpdateValues( | ||
| parallelThresholdLog2: Int, | ||
| transform: (K, V) -> R? | ||
| ): TreapMap<K, R> = treapMapOf() |
There was a problem hiding this comment.
can't you return this and just unsafe cast it? That's ultimately what treapMapOf is doing right? I'm not suggesting we do this, just making sure I understand what the code is doing.
There was a problem hiding this comment.
Yes, this is just a nicer-looking way of doing that.
| override fun <R : Any> shallowMapReduce(map: (K, V) -> R, reduce: (R, R) -> R): R { | ||
| var result: R? = null | ||
| forEachPair { | ||
| val mapped = map(it.key, it.value) | ||
| result = result?.let { result -> reduce(result, mapped) } ?: mapped | ||
| } | ||
| return result!! | ||
| } |
There was a problem hiding this comment.
is the type bound on R here just so we have nullability of result? I'm trying to think of a way to relax this, but I think you can only use, e.g., lateinit on non-null types.
Probably not worth it.
There was a problem hiding this comment.
We depend on this constraint in mapReduceImpl, where it's a bigger deal. It avoids needing to allocate a result holder for each step of the traversal.
|
|
||
| internal tailrec fun <E> TreapListNode<E>?.isApproximatelySmallerThanLog2(sizeLog2: Int): Boolean = when { | ||
| this == null -> true | ||
| sizeLog2 <= 0 -> false |
There was a problem hiding this comment.
when could this ever be less than zero? Seems like we should throw on this behavior?
| public fun <R : Any> updateValues( | ||
| transform: (K, V) -> R? | ||
| ): TreapMap<K, R> |
There was a problem hiding this comment.
So this is a change in the public API right? If V was a nullable type, you could use this function to update the values of the map. But now you can't, updatedValues is unusable for nullable types. Maybe that was a bug, as I suspect that we removed entries for which transform returned null, but still, it means part of the API is unusable depending on your type parameter.
There was a problem hiding this comment.
I think I'm favor of this change, as it makes explicit that null values are filtered out, where as previously you could write:
treapMapOf("foo" to 3, "bar" to null).updateValues { _, v -> v }and have this return a map without "bar" (and no null keys) but this wasn't reflected in the return type of updateValues
There was a problem hiding this comment.
Can we document this behavior a bit more explicitly though?
There was a problem hiding this comment.
You can definitely write this, before and after this change:
var map = treapMapOf("foo" to 3, "bar" to null).updateValues { _, v -> v }
In both cases the result is a map without the entries that had null values. With this change, the result will be typed as TreapMap<String, Int> instead of TreapMap<String, Int?>.
There was a problem hiding this comment.
I will add some comments.
While doing some Prover performance work, I found the following to be useful:
Add
mapReduceandparallelMapReducemethods onTreapSet,TreapMap, andTreapList. These do what you think they do, and are useful for the obvious reasons.Allow
TreapMap.updateValues(andparallelUpdateValues) to change the type of the values. This makes a straightforward mapping of values to different types an O(N) operation instead of O(N log N).I also simplified the
TreapMap.updateEntrysignature a bit (it had some extraneous nullability annotations).