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

Symbol not getting removed when item is removed from tree #11

Open
Ineluki opened this issue Aug 26, 2017 · 2 comments
Open

Symbol not getting removed when item is removed from tree #11

Ineluki opened this issue Aug 26, 2017 · 2 comments

Comments

@Ineluki
Copy link

Ineluki commented Aug 26, 2017

When items are repeatedly added to and removed from new trees, the symbols stay and start clogging up the memory. Don't know if this is necessary by design or not.

My simple test:

const SymbolTree = require('symbol-tree');

const base = [];
for(let i=0; i<100; i++) {
	base.push({ content: i });
}

for(let i=0; i<1000; i++) {
	let tree = new SymbolTree();
	let root = { root: true };
	base.forEach(obj => {
		tree.appendChild(root, obj);
	});

	let it = tree.childrenIterator(root);
	for( child of it ){
		tree.remove(child);
	}
	console.log(JSON.stringify(process.memoryUsage()));

}
@Joris-van-der-Wel
Copy link
Member

This was indeed by design because it was the fastest solution for jsdom's use case. In jsdom objects are often removed from the SymbolTree and then added back a little while later. (Also note that jsdom uses a finite amount of SymbolTree instances). Deleting and then adding back the property would be slower, and also delete may cause deopts in v8.

Would it help if I added an option to the constructor?

const tree = new SymbolTree({cleanup: true});
tree.remove(someObj); // which would then trigger `delete someObj[this.symbol]`

And then jsdom could continue to use the {cleanup: false} behaviour.

@Ineluki
Copy link
Author

Ineluki commented Aug 27, 2017

That would be a good solution, yes. If delete is bad in these cases, I'd just recommend setting it to null if that makes a difference.
But if it's by design, don't feel pressured on my part. Turns out I can't use this awesome lib anyway, unfortunately, since it can't allow the same object multiple times. Just thought I'd mention the problem since I already wrote the test and thought somebody might one day run into the problem ;)

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

No branches or pull requests

2 participants