Fiber Reconciler fixes #525

ahti · 2022-10-31T06:07:57Z

This is what I have done so far to make the example (with a little modification) from #523 work.

Currently, things work until returning to the initial state, after which pressing the button will leave an empty screen.

Some comments in no particular order:

The reproducer target is of course only for debugging and to be removed before merge.
I've left the logs as normal prints for now, since swift-log doesn't build on wasm rn, we can remove those as well if things get ready before swift-log is fixed.
Pay no attention to the individual commits, I'll clean those up once things look more mergeable.
The implementation of ReconcilePass.reconcile(_: , in:, caches:) obviously still needs to be refactored.
The reconciliation pass currently emits mutations that seem to assume replacing an element transfers any children to the replacement. I've adjusted the DOM and test renderers, but maybe the emitted mutations should be changed instead.
I tried turning the reproducer into a test case, but that fails one step earlier, while returning to the initial state, due to an out of range index in an insert mutation.
Another issue I observed: There are cases where a remove mutation is emitted with the wrong parent. The DOMRenderer doesn't use the parent, so it's not an issue there, but it does materialize with the test renderer.
The parent in the remove mutation is optional, but all places where such a value is created pass a non-nil parent. Is there a reason for this, or can this be turned non-optional?

carson-katri · 2022-11-01T18:23:12Z

I don't recall specifically, but the parent may be be optional because the root of the hierarchy has no parent and could hypothetically be removed.

carson-katri · 2022-11-01T18:41:35Z

Tested on a small project and didn't notice any regressions.

ahti · 2023-01-01T23:26:37Z

I finally found some time and started refactoring ReconcilePass.reconcile (not quite done with that), and also fixed the empty screen when going through all the states of the reproducer and then from .a to .b again (nothing was inserted when a fiber that should have an element had an alternate, but the alternate didn't have an element).

There is still an issue with label content not being updated tho, which I'm going to look into next.

ahti · 2023-01-02T07:10:30Z

Fixed the stale label content as well.

The only issue left that I am aware of is the .remove mutation with wrong parent. I've extended the test to reproduce that one, too. Oh and the reconcile method could still be dryed a bit further.

I also changed the test renderer so that multiple changed fibers get batched like they do in real renderers. Without that, the test was kind of flaky and would randomly succeed or fail (before adding the part to reproduce the remove bug, that is).

carson-katri

Very nice improvements, thank you. I used it on a test project of mine and it worked well.

carson-katri · 2023-01-14T20:28:29Z

Package.swift

@@ -15,6 +15,7 @@ let package = Package(
      name: "TokamakDemo",
      targets: ["TokamakDemo"]
    ),
+    .executable(name: "RecRep", targets: ["RecRep"]),


I'm not sure we should have this executable as part of the Tokamak package. It seems you have already adapted it to a test case, which should be enough.

carson-katri · 2023-01-14T20:30:03Z

Sources/TokamakCore/Fiber/FiberReconciler.swift

+    print("""
+      reconcile done
+      mutations were:
+      \(visitor.mutations.map { "  \($0)" }.joined(separator: "\n"))
+      alternate is \(self.alternate.recursiveDescription)
+      current is \(current.recursiveDescription)
+      """)


Can you clean up any print statements before merging? Better debug logging could be tackled in a separate PR.

carson-katri · 2023-01-14T20:35:50Z

Sources/TokamakCore/Fiber/Passes/ReconcilePass.swift

+
+    invalidateCache(for: fiber, in: reconciler, caches: caches)
+
+    switch (fiber.element, node.newContent, fiber.alternate?.element) {


Very nice improvement here!

carson-katri · 2023-01-14T20:38:55Z

Sources/TokamakCore/Fiber/Passes/ReconcilePass.swift

+      if let p = fiber.elementParent { caches.elementIndices[ObjectIdentifier(p)]? -= 1 }
+      return .remove(element: alt, parent: parent)
+
+    case let (element?, content, _) where fiber.alternate?.element == nil:


Couldn't this be written as:

Suggested change

case let (element?, content, _) where fiber.alternate?.element == nil:

case let (element?, content, nil):

ahti · 2023-09-03T20:50:33Z

superseeded by #542

ahti added 7 commits October 31, 2022 06:39

prints n stuff

f640e72

reproducer

2608249

wip

0700d78

wip

3d8e2f7

fixed!

c38a05a

whoops new issue

ce73da1

wip

5d5e67f

carson-katri added bug Something isn't working Fiber Changes related to the FiberReconciler or a FiberRenderer labels Nov 1, 2022

carson-katri linked an issue Nov 1, 2022 that may be closed by this pull request

Fiber layout/reconciliation issues #523

Open

ahti added 4 commits December 28, 2022 20:56

Merge main into investigate

cae82f3

check node.fiber in the beginning

7bd170d

refactor ReconcilePass.reconcile(...)

bd8de91

fix going from state a -> b after going around once in RecRep

55d8073

ahti added 2 commits January 2, 2023 07:51

update reinserted element with newContent

09aa436

update and extend test

e961334

carson-katri requested changes Jan 14, 2023

View reviewed changes

ahti added 4 commits January 23, 2023 17:50

fix wrong parent in remove mutation

514599f

clean up reproducer target

6583249

undo logging changes

0590a9e

simplify case pattern

e15de26

carson-katri changed the title ~~[wip] fix #523~~ Fiber Reconciler fixes Feb 4, 2023

carson-katri mentioned this pull request Feb 4, 2023

fix replaceChild parameter order #522

Merged

ahti mentioned this pull request Aug 16, 2023

Fiber fixes attempt 2 #542

Open

ahti closed this Sep 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fiber Reconciler fixes #525

Fiber Reconciler fixes #525

ahti commented Oct 31, 2022

carson-katri commented Nov 1, 2022

carson-katri commented Nov 1, 2022

ahti commented Jan 1, 2023

ahti commented Jan 2, 2023 •

edited

Loading

carson-katri left a comment •

edited

Loading

carson-katri Jan 14, 2023

carson-katri Jan 14, 2023

carson-katri Jan 14, 2023

carson-katri Jan 14, 2023

ahti commented Sep 3, 2023


		invalidateCache(for: fiber, in: reconciler, caches: caches)

		switch (fiber.element, node.newContent, fiber.alternate?.element) {

	case let (element?, content, _) where fiber.alternate?.element == nil:
	case let (element?, content, nil):

Fiber Reconciler fixes #525

Fiber Reconciler fixes #525

Conversation

ahti commented Oct 31, 2022

carson-katri commented Nov 1, 2022

carson-katri commented Nov 1, 2022

ahti commented Jan 1, 2023

ahti commented Jan 2, 2023 • edited Loading

carson-katri left a comment • edited Loading

Choose a reason for hiding this comment

carson-katri Jan 14, 2023

Choose a reason for hiding this comment

carson-katri Jan 14, 2023

Choose a reason for hiding this comment

carson-katri Jan 14, 2023

Choose a reason for hiding this comment

carson-katri Jan 14, 2023

Choose a reason for hiding this comment

ahti commented Sep 3, 2023

ahti commented Jan 2, 2023 •

edited

Loading

carson-katri left a comment •

edited

Loading