Skip to content

Plots.purge does not halt render loop for scene or scene2d #126

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

Closed
emackey opened this issue Dec 16, 2015 · 10 comments · Fixed by #300
Closed

Plots.purge does not halt render loop for scene or scene2d #126

emackey opened this issue Dec 16, 2015 · 10 comments · Fixed by #300
Labels
bug something broken
Milestone

Comments

@emackey
Copy link

emackey commented Dec 16, 2015

This can be seen in the test_dashboard environment, open the console and plot this:

Plotly.newPlot(Tabs.fresh(), [{x:[1,2,3], y:[2,1,2], type:'scattergl'}]);

then purge:

Plotly.Plots.purge(Tabs.getGraph())

The render loop starts throwing exceptions but keeps firing. I believe this affects 3D scenes as well.

@mdtusz
Copy link
Contributor

mdtusz commented Dec 16, 2015

Is this related to what @alexcjohnson was saying here?

@etpinard
Copy link
Contributor

@emackey Confirmed.

This situation also occurs if purge is called in a Plotly.newPlot().then.

Is this related to what @alexcjohnson was saying here?

That said, purge isn't part of the official plotly.js API and will most likely be 🔪 in v2.

What exactly are you trying to do by purging the graph div?

To clear the graph, you could instead call:

Plotly.newPlot(Tabs.getGraph(), [], {});

@etpinard
Copy link
Contributor

@emackey Alternatively you could remove the graph div element completely e.g:

Plotly.newPlot('graph', [{x:[1,2,3], y:[2,1,2], type:'scattergl'}]).then(function() {
   d3.select('#graph').remove();
});

@emackey
Copy link
Author

emackey commented Dec 17, 2015

Thanks guys, two comments:

  • I'm pretty sure this bug persists even if you avoid purge and simply call newPlot. The old render loop keeps going, near as I can tell, and potentially a new render loop may get started on top of that, depending if the new chart type called for an additional render loop.
  • I'd like to use something similar to purge or destroy to get rid of a plot that is being dismissed from the UI of a single-page web app. The plot may have been in a popup or overlay, and that's going away now, so I want to kill off the render loop, the event handlers, and indeed get rid of any complex DOM structure that is no longer needed.

OK, third comment:

  • If a chart is just sitting there on a page, do we need that render loop humming? I'm the pot calling the kettle black here, since Cesium has this exact issue too. But if there is no visible change from the previous animation frame, why spend CPU/GPU and maybe mobile battery power to keep re-rendering the same chart over and over?

@mdtusz
Copy link
Contributor

mdtusz commented Dec 17, 2015

Perhaps pause and resume methods could be useful for the gl charts? I'm not sure of how all the gl stuff is being done under the hood, but @alexcjohnson or @etpinard will have some insight.

@etpinard
Copy link
Contributor

@emackey

I'm pretty sure this bug persists even if you avoid purge and simply call newPlot.

Interesting. Putting a breakpoint in the render loop and this

Plotly.newPlot(Tabs.fresh(), [{
    type: 'scattergl',
    x: [1,2,3],
    y: [1,2,3],
    z: [2,1,2]
}])
.then(function() {
    console.log(Plotly.d3.select('canvas').size());  // => 1
})
.then(function() {
    Plotly.newPlot(Tabs.getGraph(), []);
})
.then(function() {
    console.log(Plotly.d3.select('canvas').size());  // => 0
});

catches the breakpoint only once.

If a chart is just sitting there on a page, do we need that render loop humming?

As far as I know (I'm no WebGL expert by the way), rendering WebGL objects inside a requestAnimation is standard because a gl context has no notion of state. That said, the gl-vis modules that plotly.js is built upon do optimize the rendering loop with a is-dirty flag.

@emackey
Copy link
Author

emackey commented Dec 17, 2015

Here's what I'm seeing. Open the test_dashboard in Google Chrome, then hit F12 to open Developer Tools, and switch to the Timeline tab of DevTools. Pop open the web console at the bottom if it's not already open, and load an SVG chart:

Plotly.newPlot(Tabs.fresh(), [{x:[1,2,3], y:[2,1,2]}]);

Next, click the round gray circle at the top of DevTools, to begin recording a timeline. A recording dialog pops up and counts off some seconds. After five seconds, click Finished to see the resulting timeline. At this stage it's boring:

profile1_svg

Now, let's try it with a scattergl plot.

Plotly.newPlot(Tabs.fresh(), [{x:[1,2,3], y:[2,1,2], type: 'scattergl'}]);

Record a new 5-second timeline. It looks a bit different:

profile2_gl

In the console, repeat your previous command, so as to create a new scattergl plot in a fresh Tab. It should look exactly like the previous scattergl, but record another 5 seconds and you'll see it's different:

profile3_gl2

Repeat the fresh scattergl command several more times, and you'll see the chart get worse each time. After about 5 newPlots, it looks like this:

profile4_gl8

After about 15 of them, Chrome gets angry:

WARNING: Too many active WebGL contexts. Oldest context will be lost.

Note the word active. Chrome wouldn't care if the old contexts were sitting around unused, but that's not the case here.

The good news is, there does seem to be a way to shut down easily, at least for Scene2D. There is a flag called Scene2D.stopped that can be set, which will prevent requestAnimationFrame from firing, and halt the render loop there. There doesn't seem to be an exactly corresponding flag in the 3D scene, but this flag looks like it might be roughly in the correct place.

If you can prevent requestAnimationFrame from being called for plots that are either purged or no longer shown, you can halt the render loop. Unfortunately requestAnimationFrame itself is not smart enough to know it is associated with a particular DOM node, it only works at the window level. That means it keeps going after the plot has been removed.

@etpinard
Copy link
Contributor

@emackey Thank you very much for this report.

The gl3d modules are definitely more mature than scattergl.

Unfortunately, the plot-creation logic in the plotly.js code can't directly be shared between gl3d and gl2d. In gl3d, requestAnimationFrame is made inside the scene module whereas the gl2d module don't spin up the rendering loop. (This change in architecture was made in the hope of making multiple subplots share the same gl-context.)

The point you bring up about this.glplot.contextLost is definitely worth looking into.

Moreover, Scene2d does expose a destroy method. Perhaps, it would solve this issue. Something like:

 // grab ref to scene object
var scene2d = Tabs.getGraph()._fullLayout._plots.xy._scene2d;

// destroy it
scene2d.destroy();

might be worth trying.

@mikolalysenko
Copy link
Contributor

This seems like the same problem as #132

The problem is that we are never calling scene.destroy() for either 2d or 3d WebGL plots. Oops! To fix this, we can check if the 3d or 2d plot is needed (maybe in .plot) and then if we aren't using them, take them out.

Also we should be calling these methods in .purge(). Not doing this is an oversight.

@emackey
Copy link
Author

emackey commented Feb 24, 2016

This issue is the bug report itself, and #132 is a PR offering a possible solution to this.

Keep in mind that the user is not encouraged to call .purge() him/herself, as it's been reported that "all hell breaks loose" if this is done without immediately creating a new plot. There's talk of making purge private, which sounds crazy to me.

So the problem stands: How can an app dismiss any plot from the page, without immediately creating a new plot in its place, and expect the render loop to halt?

@etpinard etpinard added this to the On-par gl2d milestone Feb 26, 2016
@etpinard etpinard added the bug something broken label Feb 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants