pipeline redis operations to decrease overhead of repeated calls

[Kallin]

made to address performance concerns raised here: #124 . Redis operations are pipelined (batched) so that only two calls total are made when saving report, instead of 2 calls per file in report.

[Kallin]

please let me know if there are any changes that would help to get this PR merged.

[Kallin]

sorry let me resolve the failing tests 1st

[Kallin]

tests should be passing now

[kbaum]

This looks great! Reducing network trips makes a ton of sense. I think it would be great to have some benchmarks on how much of a performance improvement this is. Here is an example of some benchmarks i took for saving around 3K files:

https://gist.github.com/kbaum/8dd2913db60dd99734aa875e5c1d9acc

[danmayer]

yes would need to get some benchmarks before getting this in, but as mention on the issue, I think a better approach is to avoid the N+1 calls entirely... The issue is that the data format for tracepoint and coverage are different so having a single get and put works for coverage but wouldn't as cleanly for tracepoint... I am planning to drop tracepoint support in Coverband 3, but not sure how to handle and pick what to pull in before then as all these other workarounds increase complexity and likely aren't necessary in the new path.

[danmayer]

ok with some of the other things folks brought up I am going to pause on 3.0 for a bit and see about pulling this in for a 2.0.3 release, but first I am going to try to get a benchmark incorporated that can show the value... basically we have rake benchmarks which has shown how much faster the coverage is over tracepoint, but it hasn't shown how coverage reporting is worse with large sets of files. I will modify the gist that has been used in a couple PRs to incorporate it into the benchmark suite.

[danmayer]

OK I added the benchmark rake benchmarks:redis_reporting to track improvements on reporting high numbers of files. Here is the output on the current master branch and this PR feature branch...

On master:

rake benchmarks:redis_reporting
runs benchmarks on reporting large files to redis
Warming up --------------------------------------
       store_reports     1.000  i/100ms
Calculating -------------------------------------
       store_reports      0.600  (± 0.0%) i/s -     10.000  in  16.731129s

on this PR

rake benchmarks:redis_reporting
runs benchmarks on reporting large files to redis
Warming up --------------------------------------
       store_reports     1.000  i/100ms
Calculating -------------------------------------
       store_reports      0.704  (± 0.0%) i/s -     11.000  in  15.675115s

While this is faster, it isn't a very significant speedup at least with a local redis, when reporting to a redis across network, I would expect more of an impact... Either way, I will pull this in and see about the memory store as well, as it is a few weeks out for coverband 3 I believe given some of my other workload.

[kbaum]

This is great that you added benchmarks. We might want to try this out on heroku using heroku redis. I’ve seen it’s definitely much slower in this case.

Another concern. I’m not sure how redis pipeline works but redis operates in a single thread. Does a long pipleline with thousands of writes lock up redis for other redis operations? This could be an issue if people are using the same redis instance for coverband and sidekiq for example.

[danmayer]

Yeah, I will look at improving the benchmark suite so you can run it against localhost or a remote Redis. I don't believe pipeline will lock up the Redis but will avoid much network traffic as the app won't wait for all the responses. See details here: https://redis.io/topics/pipelining @kbaum in case you think I am missing something.

[kbaum]

Re: pipelining. Reading those docs, it sounds like redis puts commands on a queue which leads me to believe other commands from other clients are being permitted so we are probably good here.

[danmayer]

yes that is my understanding as well

[Kallin]

Thanks for taking a look at this and merging so quickly!

[danmayer]

sure @Kallin I believe I will get the updated release of 2.0.3 out today or tomorrow. That includes this fix and some others that should be helpful.