search

redis / hiredis

Minimalistic C client for Redis >= 1.2
BSD 3-Clause "New" or "Revised" License
6.2k stars 1.81k forks source link

Removing SDS from hiredis. #866

Open michael-grunder opened 4 years ago

michael-grunder commented 4 years ago

We recently bumped the hiredis bundled with Redis to support PUSH messages and ran into quite a few roadblocks over our slightly divergent copies of the SDS library.

Being both projects are written in pure C there aren't a lot of good solutions to the problem. We're really left with either (a) enforcing identical versions of SDS, or (b) renaming all of Hiredis' symbols so they don't conflict in Redis.

Neither of these options is particularly good.

The thing is, Hiredis doesn't use the SDS library extensively, and in fact, it's somewhat inefficient as we rely on sdsrange (which does a memmove) when progressing through the buffer. We are likely better served using a more appropriate data structure (e.g. a ring buffer).

@yossigo Let me know what you think. I'm going to see if I can put together a proof of concept. The trickiest part is probably going to be making sure it works in both Linux and Windows.

Related #656 #184

yossigo commented 4 years ago

@michael-grunder This sounds like a very good idea. I guess the biggest problem is it's going to break backwards compatibility as sds is exposed on structs.

michael-grunder commented 4 years ago

Looks like it's not actually too bad.

That said, redisContext->obuf is an SDS string but we export it as a char*.

We might be able to avoid breaking changes by #ifdefing our way around it. I'll see what I can do once I have something functional.

masariello commented 4 years ago

Looking forward to this. The char buf[] members of the sdshdrxx structs in sds.h cause warnings when hiredis.h is #included in C++

hejohns commented 3 years ago

I've been looking into the circular buffer as per #656. I don't believe a circular buffer would be well suited for redisReader. If a large reply gets fed, exceeding the circular buffer size, the buffer will have to get reallocated anyways.

A singly linked FIFO queue would fit well here. Here's a bit of early work: link. The ABI should be okay. It's entirely opt-in. The queue implementation is from openbsd (SIMPLEQ from sys/queue.h).

@michael-grunder tell me what you think of using a FIFO.

Related, link should really get merged. While I was doing benchmarking (callgrind), seekNewline consumed all of the time for parsing large (like #656) lines. memchr uses sse on x86_64/glibc and likely most common platforms.

edit---------- This still uses sds, but the FIFO could be implemented without sds if the symbol clashing is that horrible.