observablehq / stdlib

The Observable standard library.
https://observablehq.com/@observablehq/standard-library
ISC License
967 stars 83 forks source link

Looks like a memory leak #14

Closed RealSergeyKashin closed 6 years ago

RealSergeyKashin commented 6 years ago

I’ve made a canvas animation example: https://beta.observablehq.com/@realsergeykashin/text-slide-4

Try to change params below several times. Move sliders. Notice framerate drops.

mbostock commented 6 years ago

This is not a leak in Observable, but a leak in your notebook: you’re not clearing the timer loop when the cell is re-evaluated, and cells are re-evaluated automatically by the runtime when you change a reactive input (such as by moving the slider).

The easiest way to use animation is to use a generator rather than a requestAnimationFrame loop:

{
  let i = 0;
  while (true) {
    yield ++i;
  }
}

When you use a generator, the runtime will automatically terminate the generator before restarting it with the new input values. The generator will also animate automatically at 60 FPS using requestAnimationFrame.

For another example: https://beta.observablehq.com/@mbostock/altered-world

If you still want to use a request animation frame loop, you can also use a try-finally block to terminate your timer loop when the cell is re-evaluated: https://beta.observablehq.com/@mbostock/disposing-content

mbostock commented 6 years ago

Here’s another post on managing timers:

https://beta.observablehq.com/@tmcw/observable-anti-patterns-and-code-smells

mbostock commented 6 years ago

Here’s a fork with a fix:

https://beta.observablehq.com/@mbostock/text-slide-4

The relevant parts are replacing the drawLoop method with a draw method:

draw() {
  for (let i = 0; i < this.arrSlides.length; i++) {
    let slider = this.arrSlides[i];
    slider.animate();
  }
}

And moving the loop out to the top level:

const drawer = new Drawer();
while (true) {
  drawer.draw();
  yield ctx.canvas;
}
RealSergeyKashin commented 6 years ago

Oh, great! Thanks so much, Mike!