Skip to content
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

releasing charts from the chart registry #1119

Closed
tlrobinson opened this issue Mar 17, 2016 · 12 comments
Closed

releasing charts from the chart registry #1119

tlrobinson opened this issue Mar 17, 2016 · 12 comments
Milestone

Comments

@tlrobinson
Copy link

If you have a single-page application that renders charts, throws them away, renders more charts, etc (as we do over at https://github.com/metabase/metabase) it appears dc.chartRegistry holds onto a reference to every chart ever rendered by dc.js forever (and thus the data used by every chart as well).

This seems unfortunate. I think it would be better to explicitly create a chart group object that you add charts to (or not, if you don't need that functionality).

In the meantime, is the recommended solution to just call dc.chartRegistry.deregister(chart) if you know you're done with a chart?

Or if you don't use chart groups at all is it safe to just do setTimeout(() => dc.chartRegistry.clear(), 10000) or something?

@XaserAcheron
Copy link
Contributor

Thinking on it, I may have run into this as well. I certainly ran into a similar-sounding slowdown case in my SPA which I seem to have fixed by calling dc.chartRegistry.clear and dispose()-ing all of the crossfilter dimensions.

Would be nice to have an in-library solution, of course.

@gordonwoodhull
Copy link
Contributor

Do you have any suggestions for how this could be done automatically? It seems to me that the client creates the charts and only the client will know when they should be destroyed.

I doubt that you don't use the chart groups, because the chart groups are used to implement reaction to filters.

@XaserAcheron
Copy link
Contributor

I was thinking of suggesting something along the lines of "remove the chart if another one is defined in the same chart group with the same ID", but then I got to thinking: dc doesn't (AFAIK) have a concept of "chart ID" other than its parent selector (maybe). Can you put more than one chart on the same parent? I've never tried.

@nordfjord
Copy link

Most SPA frameworks have some lifecycle hooks. A common one would be an event when a component is removed from the DOM.

just do something like this for your chart component:

component.onunload = function(){
  dc.chartRegistry.deregister(component.chart, component.chart.chartGroup());
  component.chart.dimension().dispose();
}

@gordonwoodhull a way to reliably detect whether a chart is indeed active would be something along the lines of:

_chart.redraw = function() {
  if (!document.contains(_chart.anchor())) {
    dc.chartRegistry.deregister(_chart, _chart.chartGroup());
  }
}

Which is really expensive.

@XaserAcheron
Copy link
Contributor

I'm idly curious on how the performance impact of document.contains compares to the average crossfilter operation -- i.e. is it possible that the DOM scan is negligible in comparison to crossfilter, even for 20+ charts? (Since that likely implies 20+ dimensions ;)

Huge DOMs exist, though, so an auto-deregister would likely need to be opt-in. Just a mental note to try some profiling later-ish (given the time, of course).

@gordonwoodhull
Copy link
Contributor

I think using the framework hooks is a better option here, or if you know when everything is getting replaced, dc.chartRegistry.clear() sounds safe too. I don't know of any other links to charts except for the obvious composite charts and focus/range charts.

I don't like the idea of scanning to see if charts are still on the page - it's slow and seems error-prone to me. But it might be okay if it's under an option.

@gordonwoodhull gordonwoodhull added this to the v2.1 milestone Apr 29, 2016
@gordonwoodhull gordonwoodhull changed the title chartRegistry leads to memory leaks? releasing charts from the chart registry Apr 29, 2016
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 29, 2016

@also, returning to @tlrobinson's original question, specifying the group name is equivalent to having access to the group object. Maybe a little awkward but it should serve the purpose.

I think the chart registry should be spun off into a small specialized library based on d3-dispatch (#988) and we should be able to provide a nicer interface at that level while preserving the existing legacy interface.

@electricpawentertainment

Hoooooo.

@stevenxi
Copy link

stevenxi commented Jun 6, 2020

hi @gordonwoodhull , this issue been opened 4 years ago and still not close yet. Am I right to assume the calling dc.chartRegistry.clear is not the right solution (yet)? Is there any better solution?

@gordonwoodhull
Copy link
Contributor

There are quite a few suggestions in this issue for how to release charts from the chart registry.

I still don’t see how dc.js can do this automatically and I think it’s up to the client to figure that out. So I guess I’ll close this.

@stevenxi
Copy link

stevenxi commented Jun 6, 2020

Is calling dc.chartRegistry.clear enough to release all the allocated resources?
Obviously not expecting dc.js to automatic release anything. Just wondering if there anything else need to clean manually.

thanks.

@gordonwoodhull
Copy link
Contributor

As far as I know, that should release the resources.

If you encounter any leaks, please file an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants