Fix: O(n^2) children performance. Fixes #108

project/Versions.scala

@@ -10,7 +10,7 @@ object Versions {
 
   val ScalaDomTypes = "0.15.0"
 
-  val Airstream = "0.14.0"
+  val Airstream = "0.14.1"
 
   // Testing
 

src/main/scala/com/raquo/laminar/lifecycle/InsertContext.scala

@@ -1,17 +1,25 @@
 package com.raquo.laminar.lifecycle
 
+import com.raquo.airstream.JsMap
 import com.raquo.laminar.nodes.{ChildNode, CommentNode, ParentNode, ReactiveElement}
+import org.scalajs.dom
 
 import scala.collection.immutable
 
+// #TODO[Naming] This feels more like InserterState?
+//  "Extra nodes" are more like "content nodes"
+
 // @Note only parentNode and sentinelNode are used by all Inserter-s.
 //  - Other fields may remain un-updated if they are not needed for a particular implementation.
 class InsertContext[+El <: ReactiveElement.Base](
   val parentNode: El,
   var sentinelNode: ChildNode.Base,
   var extraNodeCount: Int, // This is separate from `extraNodes` for performance
-  var extraNodes: immutable.Seq[ChildNode.Base]
-)
+  var extraNodes: immutable.Seq[ChildNode.Base] // #TODO We don't need this anymore, we only keep it for compat in 0.14.1
+) {
+
+  private[laminar] var extraNodesMap: JsMap[dom.Node, ChildNode.Base] = InsertContext.nodesToMap(extraNodes)
+}
 
 object InsertContext {
 
@@ -28,4 +36,12 @@ object InsertContext {
       extraNodes = Nil
     )
   }
+
+  private[laminar] def nodesToMap(nodes: immutable.Seq[ChildNode.Base]): JsMap[dom.Node, ChildNode.Base] = {
+    val acc = JsMap.empty[dom.Node, ChildNode.Base]
+    nodes.foreach { node =>
+      acc.set(node.ref, node)
+    }
+    acc
+  }
 }

src/main/scala/com/raquo/laminar/modifiers/ChildInserter.scala

@@ -9,7 +9,7 @@ object ChildInserter {
 
   def apply[El <: ReactiveElement.Base] (
     $child: MountContext[El] => Observable[ChildNode.Base],
-    initialInsertContext: Option[InsertContext[El]]
+    initialInsertContext: Option[InsertContext[El]] // #TODO[API] This param appears to always be `None`. Didn't I make this for some edge case requiring a Some()? I think tha logic is in `Inserter`
   ): Inserter[El] = new Inserter[El](
     initialInsertContext,
     insertFn = (c, owner) => {

src/main/scala/com/raquo/laminar/modifiers/ChildrenInserter.scala

@@ -1,5 +1,6 @@
 package com.raquo.laminar.modifiers
 
+import com.raquo.airstream.JsMap
 import com.raquo.airstream.core.{EventStream, Observable, Signal}
 import com.raquo.laminar.lifecycle.{InsertContext, MountContext}
 import com.raquo.laminar.nodes.{ChildNode, ParentNode, ReactiveElement}
@@ -29,22 +30,26 @@ object ChildrenInserter {
       }
 
       childrenSignal.foreach { newChildren =>
+        val newChildrenMap = InsertContext.nodesToMap(newChildren)
         c.extraNodeCount = updateChildren(
-          prevChildren = c.extraNodes,
+          prevChildren = c.extraNodesMap,
           nextChildren = newChildren,
+          nextChildrenMap = newChildrenMap,
           parentNode = c.parentNode,
           sentinelNode = c.sentinelNode,
           c.extraNodeCount
         )
         c.extraNodes = newChildren
+        c.extraNodesMap = newChildrenMap
       }(owner)
     }
   )
 
   /** @return New child node count */
   private def updateChildren(
-    prevChildren: Children,
+    prevChildren: JsMap[dom.Node, ChildNode.Base],
     nextChildren: Children,
+    nextChildrenMap: JsMap[dom.Node, ChildNode.Base],
     parentNode: ReactiveElement.Base,
     sentinelNode: Child,
     prevChildrenCount: Int
@@ -62,7 +67,7 @@ object ChildrenInserter {
     //    dom.console.log(">>>>>>>>>>>>>>>>>")
     //    dom.console.log(">>>>>>>>>>>>>>>>>")
 
-    nextChildren.foreach { nextChild =>
+    nextChildren.foreach { nextChild => // #TODO Not sure if this is faster than iterating over a js.Map
 
       // Desired index of `nextChild` in `liveNodeList`
       val nextChildNodeIndex = sentinelIndex + index + 1
@@ -93,7 +98,7 @@ object ChildrenInserter {
         } else {
           //          dom.console.log("NODE DOES NOT MATCH – " + nextChild.ref.textContent + " vs " + prevChildRef.textContent)
 
-          if (!prevChildren.contains(nextChild)) {
+          if (!prevChildren.has(nextChild.ref)) {
             // nextChild not found in prevChildren, so it's a new child, so we need to insert it
             // println("> new: inserting " + nextChild.ref.textContent + " at index " + nextChildNodeIndex)
             // @Note: DOM update
@@ -109,13 +114,13 @@ object ChildrenInserter {
             // - In `containsNode` call we only start looking at `index` because we know that all nodes before `index` are already in place.
             while (
               nextChild.ref != prevChildRef
-                && !containsRef(nextChildren, prevChildRef, startLookingAtIndex = index)
+                && !containsRefNew(nextChildrenMap, prevChildRef)
             ) {
               // prevChild should be deleted, so we remove it from the DOM, and try again with the next prevChild
               // but first we save its next sibling, which will become our next `prevChildRef`
               val nextPrevChildRef = prevChildRef.nextSibling //@TODO[Integrity] See warning in https://developer.mozilla.org/en-US/docs/Web/API/Node/nextSibling (should not affect us though)
 
-              val prevChild = prevChildFromRef(prevChildren, prevChildRef)
+              val prevChild = prevChildFromRefNew(prevChildren, prevChildRef)
               // println("> removing " + prevChild.ref.textContent)
               // @Note: DOM update
               ParentNode.removeChild(parent = parentNode, child = prevChild)
@@ -143,14 +148,23 @@ object ChildrenInserter {
       val nextPrevChildRef = prevChildRef.nextSibling
       // Whenever we insert, move or remove items from the DOM, we need to manually update `prevChildRef` to point to the node at the current index
       // @Note: DOM update
-      ParentNode.removeChild(parent = parentNode, child = prevChildFromRef(prevChildren, prevChildRef))
+      ParentNode.removeChild(parent = parentNode, child = prevChildFromRefNew(prevChildren, prevChildRef))
       prevChildRef = nextPrevChildRef
       currentChildrenCount -= 1
     }
 
     currentChildrenCount
   }
 
+  private def containsRefNew(nextChildrenMap: JsMap[dom.Node, ChildNode.Base], ref: dom.Node): Boolean = {
+    nextChildrenMap.has(ref)
+  }
+
+  private def prevChildFromRefNew(prevChildren: JsMap[dom.Node, ChildNode.Base], ref: dom.Node): Child = {
+    prevChildren.get(ref).get // @TODO[Integrity] Throw a meaningful error if not found (that would be unrecoverable inconsistent state)
+  }
+
+  @deprecated("0.14.1", "This is going away, see https://github.com/raquo/Laminar/issues/108")
   protected def containsRef(nextChildren: Children, ref: dom.Node, startLookingAtIndex: Int): Boolean = {
     // @TODO[Performance] This also can be optimized for different `Seq` implementations
     val childrenCount = nextChildren.size
@@ -167,11 +181,8 @@ object ChildrenInserter {
   }
 
   // @TODO[Performance] This method should not exist, I think. See how it's used, we should just have removeChildByRef method in Laminar or SDB.
+  @deprecated("0.14.1", "This is going away, see https://github.com/raquo/Laminar/issues/108")
   protected def prevChildFromRef(prevChildren: Children, ref: dom.Node): Child = {
-    //    println("> prevChildFromDomNode")
-    //    dom.console.log(prevChildren(0))
-    //    dom.console.log(prevChildren(1))
-    //    dom.console.log(ref, ref.textContent)
     prevChildren.find(_.ref == ref).get // @TODO[Integrity] Throw a more meaningful error (that would be unrecoverable inconsistent state)
   }
 }

src/main/scala/com/raquo/laminar/modifiers/Inserter.scala

@@ -11,7 +11,7 @@ import com.raquo.laminar.nodes.ReactiveElement
   * When used with onMountInsert, it "immediately" reserves an insertion
   * spot and then on every mount it inserts the node(s) into the same spot.
   *
-  * Note: As a Modifier this is not idemponent, but overall
+  * Note: As a Modifier this is not idempotent, but overall
   * it behaves as you would expect. See docs for more details.
   *
   * Note: If you DO provide initialContext, its parentNode MUST always

src/main/scala/com/raquo/laminar/nodes/ParentNode.scala

@@ -1,10 +1,12 @@
 package com.raquo.laminar.nodes
 
+import com.raquo.airstream.JsArray
 import com.raquo.airstream.ownership.DynamicOwner
 import com.raquo.laminar.DomApi
 import org.scalajs.dom
 
-import scala.collection.mutable
+import scala.scalajs.js
+import scala.scalajs.js.JSConverters._
 
 trait ParentNode[+Ref <: dom.Element] extends ReactiveNode[Ref] {
 
@@ -17,10 +19,10 @@ trait ParentNode[+Ref <: dom.Element] extends ReactiveNode[Ref] {
   //  - The only place where we need this is ReplaceAll functionality of ChildrenCommand API
   //  - We can probably track specific children affected by that API instead
   //  - That would save us mutable Buffer searches and manipulations when inserting and removing nodes
-  private var _maybeChildren: Option[mutable.Buffer[ChildNode.Base]] = None
+  private var _maybeChildren: Option[JsArray[ChildNode.Base]] = None
 
   @deprecated("ParentNode.maybeChildren will be removed in a future version of Laminar.", "0.8")
-  @inline def maybeChildren: Option[List[ChildNode.Base]] = _maybeChildren.map(_.toList)
+  @inline def maybeChildren: Option[List[ChildNode.Base]] = _maybeChildren.map(_.asInstanceOf[js.Array[ChildNode.Base]].toList)
 }
 
 object ParentNode {
@@ -52,14 +54,17 @@ object ParentNode {
 
       // 2A. Update child's current parent node
       child.maybeParent.foreach { childParent =>
-        childParent._maybeChildren.foreach(childParentChildren => childParentChildren -= child)
+        childParent._maybeChildren.foreach { children =>
+          val ix = children.indexOf(child)
+          children.splice(ix, deleteCount = 1)
+        }
       }
 
       // 2B. Update this node
       if (parent._maybeChildren.isEmpty) {
-        parent._maybeChildren = Some(mutable.Buffer(child))
+        parent._maybeChildren = Some(JsArray(child))
       } else {
-        parent._maybeChildren.foreach(children => children += child)
+        parent._maybeChildren.foreach(children => children.push(child))
       }
 
       // 3. Update child
@@ -86,7 +91,7 @@ object ParentNode {
         if (removed) {
 
           // 2. Update this node
-          children.remove(indexOfChild, count = 1)
+          children.splice(indexOfChild, deleteCount = 1)
 
           // 3. Update child
           child.setParent(None)
@@ -109,7 +114,7 @@ object ParentNode {
 
     // 0. Prep this node
     if (parent._maybeChildren.isEmpty) {
-      parent._maybeChildren = Some(mutable.Buffer())
+      parent._maybeChildren = Some(JsArray())
     }
 
     parent._maybeChildren.foreach { children =>
@@ -131,11 +136,14 @@ object ParentNode {
       if (inserted) {
         // 2A. Update child's current parent node
         child.maybeParent.foreach { childParent =>
-          childParent._maybeChildren.foreach(childParentChildren => childParentChildren -= child)
+          childParent._maybeChildren.foreach { children =>
+            val ix = children.indexOf(child)
+            children.splice(ix, deleteCount = 1)
+          }
         }
 
         // 2B. Update this node
-        children.insert(atIndex, child)
+        children.splice(atIndex, deleteCount = 0, child)
 
         // 3. Update child
         child.setParent(nextParent)
@@ -214,15 +222,17 @@ object ParentNode {
     // @TODO[Performance] introduce a reorderChildren() method to support efficient sorting?
     // @TODO[Integrity] This does not properly report failures like other methods do
 
+    val newChildrenArr = newChildren.toJSArray.asInstanceOf[JsArray[ChildNode.Base]] // #TODO
+
     // 0. Prep this node
     if (parent._maybeChildren.isEmpty) {
-      parent._maybeChildren = Some(mutable.Buffer())
+      parent._maybeChildren = Some(JsArray())
     }
 
     var replaced = false
     parent._maybeChildren.foreach { children =>
       if (
-        newChildren != children
+        newChildrenArr != children
           && fromIndex >= 0 && fromIndex < children.length
           && toIndex >= 0 && toIndex < children.length
           && fromIndex <= toIndex
@@ -239,7 +249,7 @@ object ParentNode {
 
         // B. Insert new children
         var insertedCount = 0
-        newChildren.foreach { newChild =>
+        newChildrenArr.forEach { newChild =>
           insertChild(
             parent,
             newChild,
@@ -261,7 +271,7 @@ object ParentNode {
 
     // A. Remove existing children
     parent._maybeChildren.foreach { children =>
-      children.foreach(child => removeChild(parent, child))
+      children.forEach(child => removeChild(parent, child))
     }
 
     // B. Add new children