socketry / async-redis

MIT License
83 stars 18 forks source link

Call .flush once per command #7

Closed davidor closed 5 years ago

davidor commented 5 years ago

This PR overrides Async::IO::Protocol::Line#write_line in Async::Redis::Protocol::RESP.

The original method performs a flush. This one does not and moves the responsibility of flushing to the caller of the method.

In the case of Redis, we do not want to perform a flush in every line, because each Redis command contains several lines. Flushing once per command is more efficient because it avoids unnecessary writes to the socket.

In a project that I'm working on, I noticed that, when using this library, a large % of the CPU time was spent on the write_lines method. Then I discovered that each call to that method performs a "flush" (https://github.com/socketry/async-io/blob/6b26891275d0b1b17f3edf5ce286c79c15066114/lib/async/io/protocol/line.rb#L45) which results in an IO.write (https://github.com/socketry/async-io/blob/6b26891275d0b1b17f3edf5ce286c79c15066114/lib/async/io/stream.rb#L141). After applying this patch, the % of CPU time spent on write_lines went down significantly.

ioquatix commented 5 years ago

Looks good.

ioquatix commented 5 years ago

I released in v0.3.3

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 69


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/async/redis/protocol/resp.rb 7 8 87.5%
<!-- Total: 7 8 87.5% -->
Totals Coverage Status
Change from base Build 68: -0.01%
Covered Lines: 333
Relevant Lines: 386

💛 - Coveralls