[IMP] context: group components by depth

closes #399

src/component/component.ts

@@ -46,6 +46,7 @@ interface Internal<T extends Env, Props> {
   // each component has a unique id, useful mostly to handle parent/child
   // relationships
   readonly id: number;
+  depth: number;
   vnode: VNode | null;
   pvnode: VNode | null;
   isMounted: boolean;
@@ -138,11 +139,14 @@ export class Component<T extends Env, Props extends {}> {
       QWeb.utils.validateProps(this.constructor, this.props);
     }
     let id: number = nextId++;
+    let depth;
     let p: Component<T, any> | null = null;
     if (parent instanceof Component) {
       p = parent;
       this.env = parent.env;
-      parent.__owl__.children[id] = this;
+      const __powl__ = parent.__owl__;
+      __powl__.children[id] = this;
+      depth = __powl__.depth + 1;
     } else {
       this.env = parent;
       this.env.qweb.on("update", this, () => {
@@ -158,11 +162,13 @@ export class Component<T extends Env, Props extends {}> {
           this.env.qweb.off("update", this);
         }
       });
+      depth = 0;
     }
     const qweb = this.env.qweb;
 
     this.__owl__ = {
       id: id,
+      depth: depth,
       vnode: null,
       pvnode: null,
       isMounted: false,

src/context.ts

@@ -12,6 +12,28 @@ import { onWillUnmount } from "./hooks";
  * With a `Context` object, each component can subscribe (with the `useContext`
  * hook) to its state, and will be updated whenever the context state is updated.
  */
+
+function partitionBy<T>(arr: T[], fn: (t: T) => boolean) {
+  let lastGroup: T[] | false = false;
+  let lastValue;
+  return arr.reduce((acc: T[][], cur) => {
+    let curVal = fn(cur);
+    if (lastGroup) {
+      if (curVal === lastValue) {
+        lastGroup.push(cur);
+      } else {
+        lastGroup = false;
+      }
+    }
+    if (!lastGroup) {
+      lastGroup = [cur];
+      acc.push(lastGroup);
+    }
+    lastValue = curVal;
+    return acc;
+  }, []);
+}
+
 export class Context extends EventBus {
   state: any;
   observer: Observer;
@@ -24,38 +46,47 @@ export class Context extends EventBus {
     this.observer = new Observer();
     this.observer.notifyCB = this.__notifyComponents.bind(this);
     this.state = this.observer.observe(state);
+    this.subscriptions.update = [];
   }
 
   /**
    * Instead of using trigger to emit an update event, we actually implement
    * our own function to do that.  The reason is that we need to be smarter than
    * a simple trigger function: we need to wait for parent components to be
-   * done before doing children components.  The reason is that if an update
-   * as an effect of destroying a children, we do not want to call the
-   * mapStoreToProps function of the child, nor rendering it.
+   * done before doing children components.  More precisely, if an update
+   * as an effect of destroying a children, we do not want to call any code
+   * from the child, and certainly not render it.
    *
-   * This method is not optimal if we have a bunch of asynchronous components:
-   * we wait sequentially for each component to be completed before updating the
-   * next.  However, the only things that matters is that children are updated
-   * after their parents.  So, this could be optimized by being smarter, and
-   * updating all widgets concurrently, except for parents/children.
+   * This method implements a simple grouping algorithm by depth. If we have
+   * connected components of depths [2, 4,4,4,4, 3,8,8], the Context will notify
+   * them in the following groups: [2], [4,4,4,4], [3], [8,8]. Each group will
+   * be updated sequentially, but each components in a given group will be done in
+   * parallel.
    *
-   * A potential cheap way to improve this situation is to keep track of the
-   * depth of a component in the component tree. A root component has a depth of
-   * 1, then its children of 2 and so on... Then, we can update all components
-   * with the same depth in parallel.
+   * This is a very simple algorithm, but it avoids checking if a given
+   * component is a child of another.
    */
   async __notifyComponents() {
     const rev = ++this.rev;
-    const subs = this.subscriptions.update || [];
-    for (let i = 0, iLen = subs.length; i < iLen; i++) {
-      const sub = subs[i];
-      const shouldCallback = sub.owner ? sub.owner.__owl__.isMounted : true;
-      if (shouldCallback) {
-        const render = sub.callback.call(sub.owner, rev);
-        scheduler.flush();
-        await render;
-      }
+    const subscriptions = this.subscriptions.update;
+    const groups = partitionBy(subscriptions, s => (s.owner ? s.owner.__owl__.depth : -1));
+    for (let group of groups) {
+      const proms = Promise.all(
+        group.map(sub => {
+          if (sub.owner ? sub.owner.__owl__.isMounted : true) {
+            return sub.callback.call(sub.owner, rev);
+          }
+        })
+      );
+      // at this point, each component in the current group has registered a
+      // top level fiber in the scheduler. It could happen that rendering these
+      // components is done (if they have no children).  This is why we manually
+      // flush the scheduler.  This will force the scheduler to check
+      // immediately if they are done, which will cause their rendering
+      // promise to resolve earlier, which means that there is a chance of
+      // processing the next group in the same frame.
+      scheduler.flush();
+      await proms;
     }
   }
 }

tests/context.test.ts

@@ -76,6 +76,88 @@ describe("Context", () => {
     expect(fixture.innerHTML).toBe("<div><span>321</span><span>321</span></div>");
   });
 
+  test("two async components are updated in parallel", async () => {
+    const testContext = new Context({ value: 123 });
+    const def = makeDeferred();
+    const steps: string[] = [];
+
+    class Child extends Component<any, any> {
+      static template = xml`<span><t t-esc="contextObj.value"/></span>`;
+      contextObj = useContext(testContext);
+      async render() {
+        steps.push("render");
+        await def;
+        return super.render();
+      }
+    }
+
+    class Parent extends Component<any, any> {
+      static template = xml`<div><Child /><Child /></div>`;
+      static components = { Child };
+    }
+    const parent = new Parent(env);
+    await parent.mount(fixture);
+    expect(fixture.innerHTML).toBe("<div><span>123</span><span>123</span></div>");
+    testContext.state.value = 321;
+    await nextTick();
+    expect(steps).toEqual(["render", "render"]);
+    expect(fixture.innerHTML).toBe("<div><span>123</span><span>123</span></div>");
+    def.resolve();
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><span>321</span><span>321</span></div>");
+  });
+
+  test("two async components on two levels are updated in parallel", async () => {
+    const testContext = new Context({ value: 123 });
+    const def = makeDeferred();
+    const steps: string[] = [];
+
+    class SlowComp extends Component<any, any> {
+      static template = xml`<p><t t-esc="props.value"/></p>`;
+      willUpdateProps() {
+        return def;
+      }
+    }
+    class Child extends Component<any, any> {
+      static template = xml`<span><SlowComp value="contextObj.value"/></span>`;
+      static components = { SlowComp };
+      contextObj = useContext(testContext);
+
+      render() {
+        steps.push("render");
+        return super.render();
+      }
+    }
+
+    class Parent extends Component<any, any> {
+      static template = xml`<div><Child /><Child /></div>`;
+      static components = { Child };
+    }
+
+    class App extends Component<any, any> {
+      static template = xml`<div><Child /><Parent /></div>`;
+      static components = { Child, Parent };
+    }
+
+    const app = new App(env);
+    await app.mount(fixture);
+    expect(fixture.innerHTML).toBe(
+      "<div><span><p>123</p></span><div><span><p>123</p></span><span><p>123</p></span></div></div>"
+    );
+    testContext.state.value = 321;
+    await nextTick();
+    expect(steps).toEqual(["render"]);
+    expect(fixture.innerHTML).toBe(
+      "<div><span><p>123</p></span><div><span><p>123</p></span><span><p>123</p></span></div></div>"
+    );
+    def.resolve();
+    await nextTick();
+    expect(steps).toEqual(["render", "render", "render"]);
+    expect(fixture.innerHTML).toBe(
+      "<div><span><p>321</p></span><div><span><p>321</p></span><span><p>321</p></span></div></div>"
+    );
+  });
+
   test("one components can subscribe twice to same context", async () => {
     const testContext = new Context({ a: 1, b: 2 });
     const steps: string[] = [];