Perf optimization on diffContentMultiple #317
Conversation
Convert lastList to array to speed up indexed access for large lists.
|
When dealing with large lists, the call |
|
Interesting, how many attributes does it take to make it "slow' in your case? I'm a bit reluctant to merge this as it allocates in a perf critical path. Really looking forward to https://github.com/dotnet/fsharp/pull/12859/files/5d4a2d1b35e9f4afbe3c2e3d4cf71d198dbbebfc..7acde1b39f139bdacc105ce43ca3c2f7a8b06f75 so we could use immutable arrays in the DSL from the start. |
|
Is the |
| let lastList = lastList |> List.toArray | ||
| nextList | ||
| |> List.mapi (fun index next -> | ||
| if index < lastList.Length | ||
| then Differ.diff (lastList.[index], next) | ||
| else ViewDelta.From next | ||
| ) |
There was a problem hiding this comment.
| let lastList = lastList |> List.toArray | |
| nextList | |
| |> List.mapi (fun index next -> | |
| if index < lastList.Length | |
| then Differ.diff (lastList.[index], next) | |
| else ViewDelta.From next | |
| ) | |
| nextList |> List.mapi (fun index next -> | |
| if index + 1 <= lastList.Length then | |
| Differ.diff(lastList.[index], next) | |
| else | |
| ViewDelta.From next | |
| ) |
There was a problem hiding this comment.
This should be an improvement we can merge immediately.
The difference becomes notable over 10,000 elements. This is probably not common for the typical business apps, but I have been working on a Game of Life toy implementation. At 200x200 cells, that is 40,000 elements. With the change above, this runs decently smoothly. Rough benchmark: type Change =
| Difference of int
| New of int
let newList = [ 1 .. 100_000 ]
let oldList = [ 2 .. 2 .. 100_000 ]
let original (newList: list<int>) (oldList: list<int>) =
newList
|> List.mapi (fun index next ->
if index + 1 <= oldList.Length
then Difference(oldList.[index] - newList.[index])
else New next
)
let modified (newList: list<int>) (oldList: list<int>) =
let oldList = oldList |> List.toArray
newList
|> List.mapi (fun i next ->
if i < oldList.Length
then Difference (oldList.[i] - next)
else New next
)
#time "on"
original newList oldList |> ignore
// Real: 00:00:07.884, CPU: 00:00:07.218, GC gen0: 2, gen1: 2, gen2: 1
modified newList oldList |> ignore
// Real: 00:00:00.006, CPU: 00:00:00.000, GC gen0: 0, gen1: 0, gen2: 0The Game of Life sample is a fun benchmark, I'll keep digging for hot spots. I'd like to avoid allocating a new array here, but haven't found a clean way to do it yet :) |
Would be interesting how the change suggested above (no array creation) already improves perf. |
|
I will make the formatting changes you suggested (sorry for not following the code base standard!), with one small change, keeping |
The one @Numpsy mentioned, that is let alternate (newList: list<int>) (oldList: list<int>) =
newList
|> List.mapi (fun index next ->
if index + 1 <= oldList.Length
then Difference(oldList.[index] - next)
else New next
) |
Hmm, thought there would be a bigger different if indexed collection access was the bottleneck and half of those are removed, |
|
@Numpsy I was also expecting a bigger speedup, roughly 50%. I would take the measurement with a grain of salt, running this from the scripting environment with |
|
Just in case, does it make a difference in the |
|
Good call, I tried it out quick: let alternate (newList: list<int>) (oldList: list<int>) =
let len = oldList.Length
newList
|> List.mapi (fun index next ->
if index + 1 <= len
then Difference(oldList.[index] - next)
else New next
)
|
I think we can merge this as a first improvement. I'm also happy to merge any other improvements once were sure they are safe. I don't want to merge the Just to make sure this does not have any negative effects on small lists. |
|
@JaggerJo caution is understandable, it is a key function in the loop!
|
I'm doing the same right now, let's collaborate 😄 I've pushed my stuff to main a15dc81 |
|
@mathias-brandewinder came up with this: let private diffContentMultiple (lastList: IView list, nextList: IView list) : ViewDelta list =
let lastListLength = lastList.Length
let mutable lastTail: IView list = lastList
nextList |> List.mapi (fun index next ->
if index + 1 <= lastListLength then
let result = diff(lastTail.Head, next)
lastTail <- lastTail.Tail
result
else
ViewDelta.From next
) |
|
I'd wondered if you could do something along those lines with GetEnumerator/MoveNext, but haven't tried it (perhaps less of a 'functional' approach though) |
|
@JaggerJo nice - I remember I tried this as well, but the results were poor, I think because I forgot to move the list length out of the loop. |
|
Nice! let's merge this. Hope we'll find more opportunities to improve perf like this one. |
|
Apologies for the build failure, I missed that changes had happened before my last commit. |
|
@Numpsy I tried an enumerator based approach, as far as I can tell it is no better than the current best, and the code is pretty gross, basically this: let enumeratorBased (newList: list<int>) (oldList: list<int>) =
let enumeratorOld = (oldList :> seq<int>).GetEnumerator()
let enumeratorNew = (newList :> seq<int>).GetEnumerator()
let mutable notFinished = true
[
while (enumeratorNew.MoveNext()) do
notFinished <-
if notFinished
then enumeratorOld.MoveNext()
else false
if notFinished
then
Difference (enumeratorOld.Current - enumeratorNew.Current)
else
New (enumeratorNew.Current)
] |
|
I'd been thinking more a hybrid approach like let private diffContentMultiple2 (lastList: IView list, nextList: IView list) : ViewDelta list =
let lastListLength = lastList.Length
use iter = (lastList :> IView seq).GetEnumerator()
nextList |> List.mapi (fun index next ->
if index + 1 <= lastListLength then
iter.MoveNext() |> ignore
let result = diff(iter.Current , next)
result
else
ViewDelta.From next
)but it looks like the list enumerator is implemented with head/tail itself, so it ends up being a more round about version of the suggested mutable list approach |
|
I've switched over to the fast implementation in main with this commit! I've added both of you as co-authors. Hope that's fine. BenchmarkDotNet=v0.13.5, OS=macOS Ventura 13.3 (22E252) [Darwin 22.4.0] IterationCount=5 RunStrategy=Throughput WarmupCount=5
|
Convert lastList to array to speed up indexed access for large lists.