Minor performance tweaks #158
Conversation
|
@virtuald same question here - any statistically significant improvement here? I worry about chasing micro-optimizations... |
|
Above question was w/r/t the re: the deferred read, I think the change is safe because 👍 |
|
The time.time change isn't significant. Heh, well, it's clear that whomever wrote that code originally really wanted to make sure that it defers to the ioloop, since the test checks for it. I'll fix the test later today. |
| @@ -277,7 +277,7 @@ def _read_body(self, data): | |||
| self.trigger(event.DATA, conn=self, data=data) | |||
| except Exception: | |||
| logger.exception('uncaught exception in data event') | |||
| self.io_loop.add_callback(self._start_read) | |||
There was a problem hiding this comment.
I have several spots where i use several independent reader's on the same IOLoop so yielding is important to me. It's also important because calling things directly can lead to infinite call stacks. I think that makes me 👎 for this change.
There was a problem hiding this comment.
Yeah, that seems reasonable enough. I wonder if there's a more efficient way of doing this then?
There was a problem hiding this comment.
I'm pretty sure it won't "block", it'll either immediately handle unprocessed (but read) bytes or it will defer and setup a callback.
http://www.tornadoweb.org/en/stable/_modules/tornado/iostream.html#BaseIOStream.read_bytes
This just skips an additional deferment.
There was a problem hiding this comment.
Right -- the question I have is whether it's possible that enough data could be fed in at the right rhythm that would cause it to never defer. That's something I'm a bit unsure about.
There was a problem hiding this comment.
From my scanning of the tornado code, I think it's functionally equivalent with one less deferment.
There was a problem hiding this comment.
@mreiferson thanks for digging a little further. I agree, and that addresses my previous concern.
|
Let's drop the Assuming @jehiah is amenable to the other change and we fix the failing test, this LGTM! |
|
I fixed the tests... it looks like one of the tests on Travis failed with a spurious error (connection refused to nsqd), not totally sure what caused that. |
|
restarted those individual tests, let's see if we get a 🍏 |
|
LGTM |
|
thanks! |
The first one is pretty self-explanatory (maybe needs another PR?) -- only call
time.time()once, the extra function overhead (despite how small it is) isn't necessary.The second one we can debate, in the reader it now immediately calls _start_read() instead of going through the IOLoop to call it. This results in a ~1s improvement in my rawperf script. I think I understand why it was done this way originally -- presumably don't want to starve the IOLoop if messages just happen to come in at the perfect rate where the socket buffer can keep feeding messages... since the read functions will call each other continuously in a loop without yielding to the IOLoop.
But... I wonder if that can actually happen?
If it can happen... then perhaps it should only use add_callback every once in awhile, or if it detects a cycle... or some other probably terrible thing? At the very least, should add a note as to why the code is using add_callback here.