techniq / layerchart

Composable Svelte chart components to build a wide range of visualizations
https://www.layerchart.com
MIT License
663 stars 12 forks source link

`ForceSimulation` keeps redundantly re-initializing all forces any time any of their parameters change #201

Closed regexident closed 5 months ago

regexident commented 5 months ago

ForceSimulation (when uses as in the examples) keeps redundantly(!) re-initializing all(!) forces any time any of their parameters change.

To inspect add this spying force to any of the "Force" examples:

function forceSpy(): any {
  function force(alpha: number) {
    // does nothing
  }

  force.initialize = function (_: any) {
    console.log('initializing spy force!');
  };

  return force;
}

… and plug it in like this:

<ForceSimulation
  forces={{
    /* ... */
    spy: forceSpy(),
  }}
  /* ... */
>

… then open the console and watch for "initializing spy force!" while resizing the window.

The reason for this is that the examples are passing forces={{ /* ... */ }}, which due to objects having reference semantics in Svelte's reactivity model causes an update as {…} creates a new object instance every time.

In order to avoid the re-initialization of forces a more efficient approach is to store the forces in a variable:

let forces: Record<string, Force<any, any>> = {
  /* ... */
  center: forceCenter(0, 0),
};

function forcesFor(size: { width: number; height: number }): Record<string, Force<any, any>> {
  const centerForce = forces.center as ForceCenter<any>;
  centerForce.x(size.width / 2).y(size.height / 2);
  return forces;
}

… and pass them to the component like this:

<ForceSimulation
  forces={forcesFor({ width, height })}
  /* ... */
>

The forces now get initialized only once and never re-initialized. While the simulation still works as expected. There is one caveat with this approach though: any new forces added to the cached forces object will be ignored by ForceSimulation. As such for any structural update of forces the cache object would have to get replaced in forcesFor(…). Not ideal. (Update: see comment below for a cleaner solution.)


This is less of a "bug" in ForceSimulation itself and more of a "bug" in the examples. Either way the examples should highlight the performance implications of passing in forces via {…} literal.

regexident commented 5 months ago

See https://github.com/techniq/layerchart/issues/206#issuecomment-2185150704 for a possible fix, making the use of caching of the forces object itself obsolete.

As such with the above fix one could cache individual forces (especially those with expensive initialization):

const cachedCollideForce = forceCollide(collideRadius);

… and then pass them to the component with the familiar {…} syntax without performance penalty:

<ForceSimulation
  forces={{
    // Initialization of center force is O(1), so we should be fine just passing a new one every frame:
    center: forceCenter(size.width / 2, size.height / 2),
    // Initialization of collide force is O(nodes), so we should re-use it:
    collide: cachedCollideForce
  }}
  /* ... */
>