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

Experimental "React Fiber"-like Reconciler #471

Merged
merged 30 commits into from May 23, 2022
Merged

Experimental "React Fiber"-like Reconciler #471

merged 30 commits into from May 23, 2022

Conversation

carson-katri
Copy link
Member

This PR adds a new reconciler modeled after React Fiber. It has a few potential benefits over the current stack reconciler:

  1. Non-recursive. This may help mitigate some of the stack overflow issues.
  2. Reduces use of AnyView. We can instead use a ViewVisitor to traverse the View tree without type-erasing all of the children.
  3. Mounted element updates are batched, which may reduce some of the cost of bridging to JavaScript by deferring it until all mutations have been gathered, although I'm not sure if this is the case or not.
  4. Can be used to experiment with a custom layout engine that matches SwiftUI 1-1. I've been experimenting with this in the fiber/layout branch, but it does not work all that well yet.

We will need to test it to figure out if these are actually of any benefit.

This exists alongside the current StackReconciler class and Renderer protocol as FiberReconciler and FiberRenderer. I'd like to get some feedback on this to know if its something that would be worth the effort, and whether or not I've overlooked anything major in the design.

I have only tested it with very simple Views. App and Scene are not supported, as well as preferences. However, preferences may be simple to implement with this model as we traverse down then back up the tree in the main reconciler loop.

Note: To try the DOM renderer you must pass the custom index.html to Carton, as all DOM mutations are performed on the JavaScript side.

@carson-katri carson-katri added the refactor No user-visible functionality change label Feb 21, 2022
@carson-katri carson-katri marked this pull request as draft February 21, 2022 21:31
@ezraberch
Copy link
Member

  • Non-recursive. This may help mitigate some of the stack overflow issues.

To test this, I used the following:

struct RecursiveView: View {
  let count: Int

  init(_ count: Int) {
    self.count = count
  }

  var body: some View {
    if count == 0 {
      Text("RecursiveView")
    } else {
      RecursiveView(count-1)
    }
  }
}

Results after adding it to your demo:

Current reconciler: 400 works, 500 overflows.
New reconciler: 900 works, 1000 loads but button causes overflow, 1100 overflows.

@carson-katri
Copy link
Member Author

@ezraberch That's cool, thanks for testing it out! Is that with or without the stack-size flag in Package.swift?

@ezraberch
Copy link
Member

With. If I remove that flag, the breaking point changes to around 30 for current and 700 for new.

@carson-katri carson-katri marked this pull request as ready for review March 7, 2022 21:07
@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Mar 8, 2022

Would fixing 3 remaining tests require any significant changes to this PR?

Anyway, reviewing right now as it is.

<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<title>Tokamak App</title>
<script>
window.Tokamak = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any benefit to having this snippet written in JS instead of Swift? Performance benefits for example?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carson-katri I assume you've addressed this with your benchmarks comment? Could still benefit with a code comment either making a reference to benchmarks or some other explanation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wasn't able to test that. For now, I've rewritten that in Swift. It definitely simplifies things a bit.

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how well documented this is, amazing work! 👏

Just a couple of nits in terms of naming, code comments, and file headers.

Also, did you try adding benchmarks to TokamakStaticHTMLBenchmark to compare how it performs when compared to the existing stack reconciler?

@MaxDesiatov MaxDesiatov changed the title Experimental React Fiber-like Reconciler Experimental "React Fiber"-like Reconciler Mar 8, 2022
@carson-katri
Copy link
Member Author

carson-katri commented Apr 5, 2022

I added some benchmarks to compare the two reconcilers. Here are the results I got running it.

name time std iterations
render Text (StackReconciler) 62699.000 ns ± 35.86 % 20785
render Text (FiberReconciler) 77655.000 ns ± 38.10 % 16989
render ForEach (StackReconciler) 15578671.000 ns ± 7.64 % 87
render ForEach (FiberReconciler) 4016959.000 ns ± 7.70 % 335
render RecursiveView(1000) (StackReconciler) 44926880.000 ns ± 1.82 % 31
render RecursiveView(1000) (FiberReconciler) 20088020.000 ns ± 4.34 % 69

Also calculated the differences:

name Δ
render Text +14956.000 ns
render ForEach -11561712.000 ns
render RecursiveView(100) -24838860.000 ns

Edit: Ran it again with milliseconds as the time unit if that's easier to read:

name                                         time       std        iterations
-----------------------------------------------------------------------------
render Text (StackReconciler)                  0.062 ms ±  31.10 %      21053
render Text (FiberReconciler)                  0.076 ms ±  36.13 %      17142
render ForEach(100) (StackReconciler)         15.043 ms ±   3.69 %         91
render ForEach(100) (FiberReconciler)          4.042 ms ±   9.20 %        332
render ForEach(1000) (StackReconciler)       152.446 ms ±   2.09 %          9
render ForEach(1000) (FiberReconciler)        71.860 ms ±   2.57 %         20
render RecursiveView(1000) (StackReconciler)  45.548 ms ±  25.12 %         26
render RecursiveView(1000) (FiberReconciler)  20.062 ms ±   4.46 %         67

@MaxDesiatov
Copy link
Collaborator

Splendid! I think a slight regression in render Text is acceptable, especially if it improves correctness. If not, it could be fixed in a separate PR in the future. Then it's just a matter of fixing CI, addressing comments, and I'm would be super happy to approve it 🙂

@carson-katri
Copy link
Member Author

Tried a few other benchmarks, this time with just the TestRenderers. These attempt to benchmark an update.

  1. update wide - Creates a 1000 item ForEach, then updates the last item.
  2. update narrow - Creates a 1000 item ForEach, then updates the first item.
  3. update deep - Creates a 1000 item RecursiveView, then updates the content of the innermost item.
  4. update shallow - Creates a 1000 item RecursiveView, then updates a Text before the RecursiveView.
name                             time       std        iterations
-----------------------------------------------------------------
update wide (StackReconciler)    242.446 ms ±   1.56 %          6
update wide (FiberReconciler)    160.804 ms ±   0.64 %          9
update narrow (StackReconciler)  238.905 ms ±   2.34 %          6
update narrow (FiberReconciler)  159.106 ms ±   0.84 %          9
update deep (StackReconciler)     95.278 ms ±   1.10 %         14
update deep (FiberReconciler)     38.443 ms ±   1.95 %         36
update shallow (StackReconciler)  46.097 ms ±   2.49 %         30
update shallow (FiberReconciler)  20.681 ms ±   2.99 %         66

I'm sure better benchmarks could be created to push the reconcilers. I'm interested in the performance difference when rendering to the browser, as the StackReconciler performs changes from the Swift side, whereas the FiberReconciler collects all of the mutations and sends them to JavaScript for handling.

@MaxDesiatov
Copy link
Collaborator

I've fixed build issues, but I'm not seeing anything on the screen when running TokamakDemo, I hope didn't break anything...

@MaxDesiatov MaxDesiatov requested a review from a team May 20, 2022 19:09
@MaxDesiatov
Copy link
Collaborator

Thanks for the updates! There are still a few unresolved comments, and I've requested a review from the team if there are any active team members remaining who could also have a look in the meantime.

@MaxDesiatov
Copy link
Collaborator

The test failure is most probably a bug in carton, I'll get that resolved unless you've already figured this one out 🙂

@MaxDesiatov
Copy link
Collaborator

Bug is fixed in swiftwasm/carton#350, I'm ready to merge that and tag a new version when approved.

MaxDesiatov
MaxDesiatov previously approved these changes May 23, 2022
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! 👏

@MaxDesiatov
Copy link
Collaborator

Demo code still needs to be restored, as far as I understand? I'd prefer that to be done before merging.

@carson-katri carson-katri merged commit 8177fc8 into main May 23, 2022
@carson-katri carson-katri deleted the fiber/core branch May 23, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor No user-visible functionality change
Development

Successfully merging this pull request may close these issues.

None yet

4 participants