Performance issues

[JohnyCilohokla]

I'm the main developer of HaxBot, which runs GhostfromTexas streams, our interactive is one of the most complex ones, but recently we started running into performance issues each time we got hosted by Channel One, it got to a point where I had to do something about it.

Every time that happened our bot was hitting over 1.5gb of memory used, interactive events where timing out, the whole bot slowed down to a crawl, it took 20-60s for it to reply to messages and then the bot was crashing after 2-5 minutes. Once the bot crashed it would just keep loop crashing until Channel one left. If we somehow survived the host the bot had a lot of memory leaked and the cpu usage was still high.

I have fixed this issue by replacing the interactive lib completely with a custom implementation, we were having other issues with the lib in the past so I was just not having it anymore and decided to just replace it, after that the the maximum memory usage was 900mb, no crashes, no random errors, no memory leaked (it dropped down to 200mb after the Channel one left), interactive is responsive, bot replies to chat instantly.

So while I'm not going to be switch back to this lib since there is no point as we already have everything implemented that we use (which is a subset of this lib), I do have some important information that should help with fixing it in this lib for everyone else.

The first and the biggest problem is here:

interactive-node/src/wire/Socket.ts

Lines 301 to 331 in a9ed387

	const promise = Promise.race([
	// Wait for replies to that packet ID:
	resolveOn(this, `reply:${packet.id()}`, timeout)
	.then((result: Reply) => {
	this.queue.delete(packet);
	if (result.error) {
	throw result.error;
	}
	return result.result;
	})
	.catch(err => {
	this.queue.delete(packet);
	throw err;
	}),
	// Never resolve if the consumer cancels the packets:
	resolveOn(packet, 'cancel', timeout + 1).then(() => {
	throw new CancelledError();
	}),
	// Re-queue packets if the socket closes:
	resolveOn(this, 'close', timeout + 1).then(() => {
	if (!this.queue.has(packet)) {
	// skip if we already resolved
	return null;
	}
	packet.setState(PacketState.Pending);
	return this.send(packet);
	}),
	]);

First of all there are 3 timers setup, this is a terrible idea, timers are not cheap and definitely not free. The timers are not cleaned up until they execute. Also there are 4 promises created, this could easily be replaced with 1 promise and 1 timer. Not that the timer per packet is even necessary. Another thing is the abuse of event emitters, while this is somewhat normal for node, using event emitters in critical (thousands of events) and really dynamic (the listeners are only attached for few seconds) is bad and node just can't handle it. Also the event for cancel/close could be handled by a single event that loops through all of the packets and rejects them, completely no need to have thousands of event listeners attached and removed all the time.

So here is how I have solved it:

• I use a Map to store the events (id->{resolve, reject, time}),
• No event emitters
• 1 timer (interval)
• The timer runs every 1 second and checks which packets need to be timed out
• The way I timeout the packets is by iterating over the map until I hit a packet that I don't need to reject as Map is ordered by insertion order, when that happens I break the loop (this removes the ability to have per packet timeouts but it's not like I ever used that, and if that is something that is necessary there are better way to implement that than timer per packet or 3 timers...)

Another issues is the massive path each reply packet takes, it's more or less
socket->event emitter->event emitter->object->event emitter
but it's hard to tell exactly with this many jumps.

Not to mention that sending packets fires an event on the packet, I mean I guess that might be useful in some extreme cases but that will add a good bit of overhead to each packet, then the packet goes through 3 internal functions, 1 is a one line of code that calls yet another function and then just 3 lines of code (if you remove the unnecessary lines...)

Another problem is that there is a lot of memory that is leaked or/and setup in a way that prevents garbage collection for a long time and a lot of the classes are not memory efficient, this is partially due to the abuse of event emitter but also a lot of things are event emitters (ex. Packet), the use of timers and overall there is a lot of unnecessary classes like Packet which then contains a Method, this could easily be replaced with a MethodPacket that combined both, or for best performance sending the raw data without making a class might be even better, if done right.

I could keep going with the problems but at this point I have mentioned most of the major ones. Now our use case was one of the most demanding ones but 4000 or 5000 packets shouldn't be something that kills the whole bot and once we replaced this lib it wasn't, so there is no doubt it was the main problem.

I might be a good time to consider redoing or refactoring the whole lib.

[ProbablePrime]

Thank you for your report, we'll look into this when we get the chance.