Skip to content
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

Fix for v0.55 collections diffing #775

Merged
merged 4 commits into from
Jul 15, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Rename + disallow keyed elements reuse for unkeyed controls
TimLariviere committed Jul 13, 2020
commit 351393c23f841dc54d2519f7fb1cbe2b4a7578a3
118 changes: 63 additions & 55 deletions Fabulous.XamarinForms/src/Fabulous.XamarinForms.Core/Collections.fs
Original file line number Diff line number Diff line change
@@ -43,13 +43,15 @@ module Collections =
| ValueSome _, ValueNone -> ClearCollection
| ValueSome _, ValueSome coll when (coll = null || coll.Length = 0) -> ClearCollection
| _, ValueSome coll ->
// Separate the previous elements into 3 lists
// Separate the previous elements into 4 lists
// The ones whose instances have been reused (dependsOn)
// The ones whose keys have been reused
// The ones whose keys have been reused and should be updated
// The ones whose keys have not been reused and should be discarded
// The rest which can be reused by any other element
let identicalElements = Dictionary<'T, int>()
let keyedElements = Dictionary<string, int * 'T>()
let reusableElements = ResizeArray<int * 'T>()
let discardedElements = ResizeArray<int>()
if prevCollOpt.IsSome && prevCollOpt.Value.Length > 0 then
for prevIndex in 0 .. prevCollOpt.Value.Length - 1 do
let prevChild = prevCollOpt.Value.[prevIndex]
@@ -66,8 +68,10 @@ module Collections =
match keyOf prevChild with
| ValueSome key when canReuseChildOf key ->
keyedElements.Add(key, (prevIndex, prevChild))
| _ ->
| ValueNone ->
reusableElements.Add((prevIndex, prevChild))
| ValueSome _ ->
discardedElements.Add(prevIndex)

let operations =
[ for i in 0 .. coll.Length - 1 do
@@ -103,6 +107,11 @@ module Collections =

| None ->
yield Insert (i, newChild)

// If we have discarded elements, delete them
if discardedElements.Count > 0 then
for prevIndex in discardedElements do
yield Delete prevIndex

// If we still have old elements that were not reused, delete them
if reusableElements.Count > 0 then
@@ -117,71 +126,70 @@ module Collections =
/// Reduces the operations of the DiffResult to be applicable to an ObservableCollection.
///
/// diff returns all the operations to move from List A to List B.
/// Except with ObservableCollection, we're forced to apply the changes one after the other, changing the indexes
/// Except with ObservableCollection, we're forced to apply the changes one after the other, changing the indices
/// So this algorithm compensates this offsetting
let reduceDiff (prevCollLength: int) (diffResult: DiffResult<'T>) : DiffResult<'T> =
let adaptDiffForObservableCollection (prevCollLength: int) (diffResult: DiffResult<'T>) : DiffResult<'T> =
match diffResult with
| NoChange -> NoChange
| ClearCollection -> ClearCollection
| Operations operations ->
let prevIndexes = Array.init prevCollLength id
let prevIndices = Array.init prevCollLength id

// Shift all old indices by 1 (down the list) on insert after the inserted position
let shiftForInsert index =
for i in 0 .. prevIndices.Length - 1 do
if prevIndices.[i] >= index then
prevIndices.[i] <- prevIndices.[i] + 1

// Shift all old indices by -1 (up the list) on delete after the deleted position
let shiftForDelete originalIndexInPrevColl prevIndex =
for i in 0 .. prevIndices.Length - 1 do
if prevIndices.[i] > prevIndex then
prevIndices.[i] <- prevIndices.[i] - 1
prevIndices.[originalIndexInPrevColl] <- -1

// Shift all old indices between the previous and new position on move
let shiftForMove originalIndexInPrevColl prevIndex newIndex =
for i in 0 .. prevIndices.Length - 1 do
if prevIndex < prevIndices.[i] && prevIndices.[i] <= newIndex then
prevIndices.[i] <- prevIndices.[i] - 1
else if newIndex <= prevIndices.[i] && prevIndices.[i] < prevIndex then
prevIndices.[i] <- prevIndices.[i] + 1
prevIndices.[originalIndexInPrevColl] <- newIndex

// Return an update operation preceded by a move only if actual indices don't match
let moveAndUpdate oldIndex prev newIndex curr =
let prevIndex = prevIndices.[oldIndex]
if prevIndex = newIndex then
Update (newIndex, prev, curr)
else
shiftForMove oldIndex prevIndex newIndex
MoveAndUpdate (prevIndex, prev, newIndex, curr)

let operations =
[ for op in operations do
match op with
| Insert (index, element) ->
yield Insert (index, element)
shiftForInsert index

for i in 0 .. prevIndexes.Length - 1 do
let prevIndex = prevIndexes.[i]
if prevIndex >= index then
prevIndexes.[i] <- prevIndexes.[i] + 1

| Move (prevIndex, newIndex) ->
if prevIndexes.[prevIndex] <> newIndex then
yield (Move (prevIndexes.[prevIndex], newIndex))

for i in 0 .. prevIndexes.Length - 1 do
if prevIndexes.[prevIndex] < prevIndexes.[i] && prevIndexes.[i] <= newIndex then
prevIndexes.[i] <- prevIndexes.[i] - 1
else if newIndex <= prevIndexes.[i] && prevIndexes.[i] < prevIndexes.[prevIndex] then
prevIndexes.[i] <- prevIndexes.[i] + 1
prevIndexes.[prevIndex] <- newIndex
| Move (oldIndex, newIndex) ->
// Prevent a move if the actual indices match
let prevIndex = prevIndices.[oldIndex]
if prevIndex <> newIndex then
yield (Move (prevIndex, newIndex))
shiftForMove oldIndex prevIndex newIndex

| Update (index, prev, curr) ->
let realPrevIndex = prevIndexes.[index]
if realPrevIndex <> index then
yield MoveAndUpdate (realPrevIndex, prev, index, curr)

for i in 0 .. prevIndexes.Length - 1 do
if prevIndexes.[index] < prevIndexes.[i] && prevIndexes.[i] <= index then
prevIndexes.[i] <- prevIndexes.[i] - 1
else if index <= prevIndexes.[i] && prevIndexes.[i] < prevIndexes.[index] then
prevIndexes.[i] <- prevIndexes.[i] + 1
prevIndexes.[index] <- index
else
yield Update (index, prev, curr)
yield moveAndUpdate index prev index curr

| MoveAndUpdate (prevIndex, prev, newIndex, curr) ->
let realPrevIndex = prevIndexes.[prevIndex]
if realPrevIndex = newIndex then
yield Update (newIndex, prev, curr)
else
yield MoveAndUpdate (realPrevIndex, prev, newIndex, curr)

for i in 0 .. prevIndexes.Length - 1 do
if prevIndexes.[prevIndex] < prevIndexes.[i] && prevIndexes.[i] <= newIndex then
prevIndexes.[i] <- prevIndexes.[i] - 1
else if newIndex <= prevIndexes.[i] && prevIndexes.[i] < prevIndexes.[prevIndex] then
prevIndexes.[i] <- prevIndexes.[i] + 1
prevIndexes.[prevIndex] <- newIndex
| MoveAndUpdate (oldIndex, prev, newIndex, curr) ->
yield moveAndUpdate oldIndex prev newIndex curr

| Delete index ->
yield Delete prevIndexes.[index]

for i in 0 .. prevIndexes.Length - 1 do
let prevIndex = prevIndexes.[i]
if prevIndex >= index then
prevIndexes.[i] <- prevIndexes.[i] - 1 ]
| Delete oldIndex ->
let prevIndex = prevIndices.[oldIndex]
yield Delete prevIndex
shiftForDelete oldIndex prevIndex ]

if operations.Length = 0 then
NoChange
@@ -204,8 +212,8 @@ module Collections =

let diffResult =
diff aggressiveReuseMode prevCollOpt collOpt keyOf canReuse
|> reduceDiff (match prevCollOpt with ValueNone -> 0 | ValueSome c -> c.Length)
|> adaptDiffForObservableCollection (match prevCollOpt with ValueNone -> 0 | ValueSome c -> c.Length)

match diffResult with
| NoChange -> ()
| ClearCollection -> targetColl.Clear()
Original file line number Diff line number Diff line change
@@ -1845,7 +1845,7 @@
"source": null,
"name": "Items",
"inputType": "ViewElement list",
"updateCode": "Collections.updateItemsViewOfTItems<'T>"
"updateCode": "Collections.updateItemsViewOfTItems<Xamarin.Forms.Cell>"
}
],
"primaryConstructorMembers": [
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
namespace Fabulous.XamarinForms.Tests.Collections

open Fabulous
open Fabulous.XamarinForms
open Fabulous.XamarinForms.Collections
open NUnit.Framework
open FsUnit

module AdaptDiffTests =
[<Test>]
let ``Test Reduce``() =
let previous =
[ View.Button(key = "Button0_0")
View.Button(key = "Button0_1")
View.Button(key = "Button0_2")
View.Button(key = "Button1_0")
View.Button(key = "Button1_1")
View.Button(key = "Button1_2")
View.Button(key = "Button2_0")
View.Button(key = "Button2_1")
View.Button(key = "Button2_2") ]

let current =
[ View.Button(key = "Button0_0")
View.Button(key = "Button0_1")
View.Button(key = "Button0_2")
View.Button(key = "Button1_0")
View.Image(key = "Image1_1")
View.Button(key = "Button1_2")
View.Button(key = "Button2_0")
View.Button(key = "Button2_1")
View.Button(key = "Button2_2") ]

let diffResult = DiffResult<ViewElement>.Operations [
Update (0, previous.[0], current.[0])
Update (1, previous.[1], current.[1])
Update (2, previous.[2], current.[2])
Update (3, previous.[3], current.[3])
Insert (4, current.[4])
Update (5, previous.[5], current.[5])
Update (6, previous.[6], current.[6])
Update (7, previous.[7], current.[7])
Update (8, previous.[8], current.[8])
Delete 4
]

Collections.adaptDiffForObservableCollection previous.Length diffResult
|> should equal (DiffResult<ViewElement>.Operations [
Update (0, previous.[0], current.[0])
Update (1, previous.[1], current.[1])
Update (2, previous.[2], current.[2])
Update (3, previous.[3], current.[3])
Insert (4, current.[4])
MoveAndUpdate (6, previous.[5], 5, current.[5])
MoveAndUpdate (7, previous.[6], 6,current.[6])
MoveAndUpdate (8, previous.[7], 7, current.[7])
MoveAndUpdate (9, previous.[8], 8, current.[8])
Delete 9
])

[<Test>]
let ``Test Reduce 2``() =
let previous =
[ View.Button(key = "Button0_0")
View.Button(key = "Button0_1")
View.Button(key = "Button0_2")
View.Button(key = "Button1_0")
View.Image(key = "Image1_1") ]

let current =
[ View.Label(key = "Button0_0")
View.Button(key = "Button0_1")
View.Image(key = "Button0_2") ]

let diffResult = DiffResult<ViewElement>.Operations [
Insert (0, current.[0])
Update (1, previous.[1], current.[1])
MoveAndUpdate (4, previous.[4], 2, current.[2])
Delete 0
Delete 2
Delete 3
]

Collections.adaptDiffForObservableCollection previous.Length diffResult
|> should equal (DiffResult<ViewElement>.Operations [
Insert (0, current.[0])
MoveAndUpdate (2, previous.[1], 1, current.[1])
MoveAndUpdate (5, previous.[4], 2, current.[2])
Delete 3
Delete 3
Delete 3
])

[<Test>]
let ``Test Reduce 3``() =
let previous =
[ View.Label(key = "0")
View.Label()
View.Button()
View.Button(key = "2")
View.Label()
View.Label(key = "3")
View.Label()
View.Button()
View.Button()
View.Button()
View.Button() ]

let current =
[ View.Button(key = "0")
View.Label() ]

let diffResult = DiffResult<ViewElement>.Operations [
MoveAndUpdate (2, previous.[2], 0, current.[0])
MoveAndUpdate (0, previous.[0], 1, current.[1])
Delete 1
Delete 3
Delete 4
Delete 5
Delete 6
Delete 7
Delete 8
Delete 9
Delete 10
]

Collections.adaptDiffForObservableCollection previous.Length diffResult
|> should equal (DiffResult<ViewElement>.Operations [
MoveAndUpdate (2, previous.[2], 0, current.[0])
Update (1, previous.[0], current.[1])
Delete 2
Delete 2
Delete 2
Delete 2
Delete 2
Delete 2
Delete 2
Delete 2
Delete 2
])



Original file line number Diff line number Diff line change
@@ -248,7 +248,8 @@ module DiffTests =

testUpdateChildren (ValueSome previous) (ValueSome current)
|> should equal (DiffResult<ViewElement>.Operations [
Update (0, previous.[0], current.[0])
Insert (0, current.[0])
Delete 0
])

/// Removing elements in the middle of others (not the same instances) should reuse the existing controls based
@@ -304,8 +305,9 @@ module DiffTests =
testUpdateChildren (ValueSome previous) (ValueSome current)
|> should equal (DiffResult<ViewElement>.Operations [
MoveAndUpdate (2, previous.[2], 0, current.[0])
Update (1, previous.[1], current.[1])
Insert (1, current.[1])
MoveAndUpdate (0, previous.[0], 2, current.[2])
Delete 1
])

/// Complex use cases with reordering and remove/add of keyed elements should reuse controls efficiently
@@ -371,7 +373,8 @@ module DiffTests =

testUpdateChildren (ValueSome previous) (ValueSome current)
|> should equal (DiffResult<ViewElement>.Operations [
Update (0, previous.[0], current.[0])
Insert (0, current.[0])
Delete 0
])

/// Replacing a non-keyed element with another when a keyed element is present should reuse the discarded element
@@ -543,4 +546,67 @@ module DiffTests =
Update (7, previous.[7], current.[7])
Update (8, previous.[8], current.[8])
Delete 4
])

[<Test>]
let ``Test Random``() =
let previous =
[ View.Button(key = "Button0_0")
View.Button(key = "Button0_1")
View.Button(key = "Button0_2")
View.Button(key = "Button1_0")
View.Image(key = "Image1_1") ]

let current =
[ View.Label(key = "Button0_0")
View.Button(key = "Button0_1")
View.Image(key = "Button0_2") ]

testUpdateChildren (ValueSome previous) (ValueSome current)
|> should equal (DiffResult<ViewElement>.Operations [
Insert (0, current.[0])
Update (1, previous.[1], current.[1])
Insert (2, current.[2])
Delete 0
Delete 2
Delete 3
Delete 4
])

[<Test>]
let ``Test Random 2``() =
let previous =
[ View.Label(key = "0")
View.Label()
View.Button()
View.Button(key = "2")
View.Label()
View.Label(key = "3")
View.Label()
View.Button()
View.Button()
View.Button()
View.Button() ]

let current =
[ View.Button(key = "0")
View.Label() ]

testUpdateChildren (ValueSome previous) (ValueSome current)
|> should equal (DiffResult<ViewElement>.Operations [
MoveAndUpdate (2, previous.[2], 0, current.[0])
Update (1, previous.[1], current.[1])

// Discarded elements = had a key that was not reused
Delete 0
Delete 3
Delete 5

// Not reused elements
Delete 4
Delete 6
Delete 7
Delete 8
Delete 9
Delete 10
])

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
namespace Fabulous.XamarinForms.Tests.Collections

open System.Collections.ObjectModel
open Fabulous
open Fabulous.XamarinForms
open Fabulous.XamarinForms.Collections
open NUnit.Framework
open FsUnit

module UpdateChildrenTests =
[<Test>]
let ``Test UpdateChildren``() =
let previous =
[ View.Button(key = "Button0_0")
View.Button(key = "Button0_1")
View.Button(key = "Button0_2")
View.Button(key = "Button1_0")
View.Image(key = "Image1_1") ]

let current =
[ View.Label(key = "Button0_0")
View.Button(key = "Button0_1")
View.Image(key = "Button0_2") ]

let collection = ObservableCollection<Xamarin.Forms.Element>()
collection.Add(Xamarin.Forms.Button())
collection.Add(Xamarin.Forms.Button())
collection.Add(Xamarin.Forms.Button())
collection.Add(Xamarin.Forms.Button())
collection.Add(Xamarin.Forms.Image())

Collections.updateChildren
(ValueSome (Array.ofList previous))
(ValueSome (Array.ofList current))
collection
(fun x -> x.Create() :?> Xamarin.Forms.Element)
(fun _ _ _ -> ())
(fun _ _ _ -> ())

let x = collection
()

Original file line number Diff line number Diff line change
@@ -10,7 +10,8 @@
<Compile Include="ViewHelpersTests.fs" />
<Compile Include="ViewConvertersTests.fs" />
<Compile Include="Collections\DiffTests.fs" />
<Compile Include="Collections\ReduceDiffTests.fs" />
<Compile Include="Collections\AdaptDiffTests.fs" />
<Compile Include="Collections\UpdateChildrenTests.fs" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="FSharp.Core" />
13 changes: 8 additions & 5 deletions docs/Fabulous.XamarinForms/views-perf.md
Original file line number Diff line number Diff line change
@@ -109,14 +109,15 @@ For all of the above, the typical, naive implementation of the `view` function r
instance on each invocation. The incremental update of dynamic views maintains a corresponding mutable target
(e.g. the `Children` property of a `Xamarin.Forms.StackLayout`, or an `ObservableCollection` to use as an `ItemsSource` to a `ListView`) based on the previous (PREV) list and the new (NEW) list.

Fabulous first prioritizes the reuse of the same ViewElement instances, when using dependsOn for instance.
Fabulous prioritizes reuse in the following order:
1. Same ViewElement instance (when using dependsOn)
```fsharp
View.Grid([
dependsOn () (fun _ _ -> View.Label(text = "Hello, World!"))
])
```

Then, it will try to reuse ViewElements sharing the same key, if `canReuseView` returns `true`.
2. Same key and control type (aka. `canReuseView` returns true)

```fsharp
// Previous View
@@ -125,16 +126,18 @@ View.Grid([
View.Label(key = "body", text = "Previous body")
])
// New View
View.Grid([
View.Label(key = "header", text = "New Header") // Will reuse previous header
View.Button(key = "body", text = "New body") // Won't be able to reuse previous body since Label != Button
])
```

If there's no matching instance or key, it will try to reuse one of the remaining previous elements to find the first one for which `canReuseView` returns `true`.
If it finds one, it reuses it, if not it creates a new control.
3. If none of the above, Fabulous will select the first element that returns `canReuseView = true` among the eligible remaining previous elements.

4. If no previous element can be reused, a new one is created

Note that old keyed elements that didn't had a matching key in the new list will be destroyed instead of being reused by new unkeyed elements to help developers avoid undesired animations, such as fade-in/fade-out on Button Text changes on iOS ([#308](https://github.com/fsprojects/Fabulous/issues/308)) or ripple effects on Android Button.

In the end, controls that weren't reused are destroyed.