Warning fixes

[jessie-murray]

I fixed a few compilation warnings: 2 in conf.c from the recent changes I made there, and also in dict.c where some functions are unused and cause a warning. I noticed also that hiredis was from an old import, maybe it should be upgraded?

1. Remove unused size_t sz variable
2. Remove const on free()'d variable
3. Remove unused function warning in dict.c

[nicolasff]

Thanks @jessie-murray!

The CircleCI build shows another one that might be good to add. I don't know if you have access to the output, so here it is again:

src/formats/msgpack.c: In function 'msgpack_wrap_redis_reply':
src/formats/msgpack.c:170:2: warning: passing argument 2 of 'msgpack_packer_new' from incompatible pointer type [enabled by default]
  msgpack_packer* pk = msgpack_packer_new(out, on_msgpack_write);
  ^
In file included from /usr/include/msgpack.h:25:0,
                 from src/formats/msgpack.h:4,
                 from src/formats/msgpack.c:1:
/usr/include/msgpack/pack.h:124:24: note: expected 'msgpack_packer_write' but argument is of type 'int (*)(void *, const char *, size_t)'
 inline msgpack_packer* msgpack_packer_new(void* data, msgpack_packer_write callback)
                        ^

You may not have seen it if you're building Webdis on a machine without msgpack installed, since the Makefile detects it and adds -DMSGPACK=1 if the library is present.

[nicolasff]

TIL about -Wimplicit-fallthrough!

-Wimplicit-fallthrough=0 disables the warning altogether.
-Wimplicit-fallthrough=1 matches .* regular expression, any comment is used as fallthrough comment.
-Wimplicit-fallthrough=2 case insensitively matches .*falls?[ \t-]*thr(ough|u).* regular expression.

Values of 3 or more use even more complex regular expressions 🤨

[nicolasff]

Looks great as always, thanks!

[nicolasff]

@jessie-murray oh and regarding your earlier comment about hiredis: I had tried to upgrade it in the past and had some weird issues so I had given up. You're welcome to give it a shot but it was not trivial from what I remember.

[jessie-murray]

@nicolasff I looked into upgrading hiredis yesterday with the thought that maybe a more recent version would be faster than the code that's currently checked in. I couldn't see any measurable difference in the request rate or latency, and the CPU usage was also similar in both versions. I didn't compare memory usage, now that I think about it it might be different.

I ran a modified version of the ab command taken from bench.sh, just with more clients and more requests to create sustained load over a longer time span. The difference was something like 91,600 requests per second before the change and 91,400 after the change which I interpreted as being the same. That said when I started the test these numbers were much lower with maybe 19,000 requests per second only, and it's only when I reduced the log level to zero that it became faster. I noticed that Webdis calls fsync after each log line, and I think this is probably what is causing the performance to drop. Maybe the fsync could be removed or made optional since this is just a log? Another way to improve performance would be to buffer log events and have a thread flushing them in batches, but this would require a much larger change. I'll look into making the fsync optional to see what kind of difference it makes.

[nicolasff]

@jessie-murray oh wow ok… I didn't remember this being the case, I'll check out the difference it makes. As for a change to change the fsync behavior, you're more than welcome to submit a PR since I won't be able to work on it. That said, I'm not convinced that adding a buffering system should be the first change; it seems like it would need a lot of code with all the potential for bugs that comes with this kind of rewrite of the logging system – which is what it feels like. This kind of change needs to demonstrate a clear advantage due to its high risk of introducing bugs.

Before exploring the buffering option I would suggest looking at the way Cassandra configures its fsync frequency with commitlog_sync. This is for actual data written to this database though, so I'm not sure whether it would make sense to have the same options for "just" log events but it's a pretty flexible approach which might work here.