Skip to content

Commit

Permalink
fix(item-set): don't leak memory when setting groups
Browse files Browse the repository at this point in the history
This is my attempt at solving #183.

There are two key differences when compared to #235:
1. This one actually works better.
2. This one doesn't blindly throw away all listeners.

Leaks in my tests (very rough measurements on my device):
- Upstream: 3 MB/s
- #235: 250 KB/s
- This: about 0 KB/s

PS: For the tests I used the JSFiddle MWE from @aphix. I just decreased
delays to leak more memory in shorter amount of time.
  • Loading branch information
Thomaash committed Feb 13, 2020
1 parent db87dfc commit f24b505
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 9 deletions.
14 changes: 14 additions & 0 deletions lib/timeline/Timeline.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,20 @@ export default class Timeline extends Core {
}
}

// This looks weird but it's necessary to prevent memory leaks.
//
// The problem is that the DataView will exist as long as the DataSet it's
// connected to. This will force it to swap the groups DataSet for it's own
// DataSet. In this arrangement it will become unreferenced from the outside
// and garbage collected.
//
// IMPORTANT NOTE: If `this.groupsData` is a DataView was created in this
// method. Even if the original is a DataView already a new one has been
// created and assigned to `this.groupsData`. In case this changes in the
// future it will be necessary to rework this!!!!
if (this.groupsData instanceof DataView) {
this.groupsData.setData(null);
}
this.groupsData = newDataSet;
this.itemSet.setGroups(newDataSet);
}
Expand Down
31 changes: 27 additions & 4 deletions lib/timeline/component/Group.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ class Group {
this.isVisible = null;
this.stackDirty = true; // if true, items will be restacked on next redraw

// This is a stack of functions (`() => void`) that will be executed before
// the instance is disposed of (method `dispose`). Anything that needs to be
// manually disposed off before garbage collection happens or so that
// garbage collection can happen should be added to this stack.
this._disposeCallbacks = [];

if (data && data.nestedGroups) {
this.nestedGroups = data.nestedGroups;
if (data.showNested == false) {
Expand Down Expand Up @@ -83,10 +89,14 @@ class Group {
byEnd: []
};
this.checkRangedItems = false; // needed to refresh the ranged items if the window is programatically changed with NO overlap.
const me = this;
this.itemSet.body.emitter.on("checkRangedItems", () => {
me.checkRangedItems = true;
})

const handleCheckRangedItems = () => {
this.checkRangedItems = true;
};
this.itemSet.body.emitter.on("checkRangedItems", handleCheckRangedItems);
this._disposeCallbacks.push(() => {
this.itemSet.body.emitter.off("checkRangedItems", handleCheckRangedItems);
});

this._create();

Expand Down Expand Up @@ -1136,6 +1146,19 @@ class Group {
this._addToSubgroup(item, newSubgroup);
this.orderSubgroups();
}

/**
* Call this method before you lose the last reference to an instance of this.
* It will remove listeners etc.
*/
dispose() {
this.hide();

let disposeCallback;
while ((disposeCallback = this._disposeCallbacks.pop())) {
disposeCallback();
}
}
}

export default Group;
9 changes: 4 additions & 5 deletions lib/timeline/component/ItemSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ class ItemSet extends Component {
if (this.groupsData) {
// remove the group holding all ungrouped items
if (ungrouped) {
ungrouped.hide();
ungrouped.dispose();
delete this.groups[UNGROUPED];

for (itemId in this.items) {
Expand Down Expand Up @@ -1289,13 +1289,12 @@ class ItemSet extends Component {
* @private
*/
_onRemoveGroups(ids) {
const groups = this.groups;
ids.forEach(id => {
const group = groups[id];
const group = this.groups[id];

if (group) {
group.hide();
delete groups[id];
group.dispose();
delete this.groups[id];
}
});

Expand Down

0 comments on commit f24b505

Please sign in to comment.