Protocol delay_between_messages hindering server performance

[mvollrath]

The Protocol is sleeping for a (default) 10ms after sending each message from the graph to its outgoing method. This is causing a very severe performance problem.

Affecting code looks like this:

rosbridge_suite/rosbridge_library/src/rosbridge_library/protocol.py

Lines 247 to 248 in d7138dc

	self.outgoing(serialized)
	time.sleep(self.delay_between_messages)

I'm working on a distributed rosbridge app with at least five client connections at the same time, subscribing to some of the same topics, across different hosts. I tried adding a two-line monkeypatch to rosbridge_websocket.py, the gist of it was:

from rosbridge_library.protocol import Protocol
Protocol.delay_between_messages = 0

This sets the default delay_between_messages to zero for new instances of Protocol. I verified this was being applied with some debug logging in the websocket handler.

The performance boost was tremendous. I removed all throtte/queue from client code and it ran silky smooth. I compared this with the default behavior (sleep 10ms after every outgoing) and it showed the signs I'd come to expect from rosbridge under even a small amount of load: messages backing up and slowing down the websocket stream.

The server did use twice as much CPU, but only went from 25-50% aggregate across all threads. I still expect Tornado to bog down under extreme load, but have yet to hit its limit since the native masking speedups were added to rosbridge_server.

With delay_between_messages completely bypassed, the experience of using robridge changes from "I'd better throttle and queue every single topic" to "let 'er rip".

Why is this happening? I haven't read all of the code, but can prepose that the effect of this delay is multiplied by the number of clients, since a message on one topic (in its own rospy subscription Thread) potentially causes the sleep once per client per message.

There is an undocumented Protocol parameter "message_intervall" that allows for setting delay_between_messages per-Protocol instance. This is not implemented in roslibjs.

rosbridge_suite/rosbridge_library/src/rosbridge_library/protocol.py

Lines 181 to 182 in d7138dc

	if "message_intervall" in msg.keys() and is_number(msg["message_intervall"]):
	self.delay_between_messages = msg["message_intervall"]

I'd like to default delay_between_messages to zero, unless there is a compelling case not to. Selective throttling and queuing works great when you need it. The only effects of delay_between_messages > 0 are backlogged, late messages that never get dropped from a queue and a Tornado IOLoop that is sitting around bored while this is happening.

[mvollrath]

Should consider, for the 2016 distro, deprecating that undocumented "message_intervall" key since it seems to be completely superceded by throttle/queue opts.

[T045T]

Good catch!
Defaulting `delay_between_messages`` to zero seems reasonable, but I'm not sure it's entirely replaced by throttle/queue. In particular, I don't think those apply to the fragments of a single message, do they?

I agree we should get rid of "message_intervall", though. Maybe we should replace it with something like fragment_interval to handle the special case of large, fragmented messages?

Also, as long as `delay_between_messages`` is still there, it should be available as a ROS parameter. It is for the TCP version of rosbridge, but nowhere else. I'll make a PR to fix that :)