Devendoring about valkey HOT 17 OPEN

Hey!

I sent a pair of MRs to KeyDB to unvendor some of its dependencies (Snapchat/KeyDB#803 and Snapchat/KeyDB#807), which I guess this might apply cleanly or might be useful as a starting point.

from valkey.

Sure I'll see if I can work up a PR for this.

Is anything besides lua modified?

from valkey.

Hi, https://github.com/terrablue/zkv contains a proof of concept of building redis with Zig, including most of the dependencies devendored. I haven't done jemalloc yet, but that's the last big one.

from valkey.

Ah, thanks for the hint! From what I can see the only thing that might need upstreaming is iget_defrag_hint(), get_defrag_hint() and the macro to signal its availability. It would be nice if the original author did that, which would mean we can use the defragging support in all redis implementations and forks. I guess barring that someone else could take it upstream, but at least I've not even looked properly at jemalloc nor the function at hand, so I'd not feel comfortable doing that.

from valkey.

Regarding hiredis #192

from valkey.

@guillemj Regarding jemalloc, I believe we need to upstream this commit by Oran Agra: 29d7f97

It's been discussed here jemalloc/jemalloc#566.

from valkey.

It's a good idea, worth considering. All deps used to be vendored, which made it really easy to build the project (just make). I guess that was the reason for having it this way.

Since TLS was added, OpenSSL is an external dependency (although optional), and TLS is becoming increasingly popular for database connections, so we already have external dependencies and might as well have more.

The vendored deps we're talking about are

• Lua. Without a vendored Lua, it may become easier to link against another Lua version, such as LuaJIT, which apparently is completely ABI-compatible and linkable even to a program built for regular Lua 5.1.
• Jemalloc. We have some local patches to Jemalloc though to be able to do active defragmentation. We'll have to look into that.
• Hiredis.
• fpconv - Very small library where I believe we only use a small part. Perhaps not important to unvendor?
• hdr_histogram - Also very small, perhaps not important to unvendor?
• The rest under deps/ is not really dependencies.

from valkey.

Yeah, I think we might consider two distributions, one with vended dependencies and one without. The only material one is the LUA as was mentioned.

from valkey.

@madolson sure we can have two distributions, but the repo would have to be without the vendored stuff, right?

We'll need makefile variables (etc) to point out where the deps are and some script to prepared the vendored release. With those in place, the user can also run that by themselves, so i don't really see the point of a release with vendored deps. Please explain why it's important.

from valkey.

There doesn't have to be two different distributions, just compile-time flags to allow using system libs instead of included ones.

from valkey.

OK @jonathanspw! Flags we can probably arrange easily. Do you want to contribute them?

We're using a simple makefile, so no fancy autodetection. Are you fine with providing full paths as variables to make?

from valkey.

I don't know if Lua is modified, but jemalloc is modified IIRC. There are some readmes under deps/ which may tell something.

from valkey.

Lua is modified, we made some changes to allow for the special global protections.

from valkey.

Cool. Just be aware of the changes to jemalloc are to enable active defragmentation. It's described in deps/REAMDE.md#jemalloc. I don't know if it's possible to upstream this to jemalloc in some way, to make active defrag work in system version.

from valkey.

@guillemj Regarding jemalloc, I believe we need to upstream this commit by Oran Agra: 29d7f97

It's been discussed here jemalloc/jemalloc#566.

@zuiderkwast, I took a quick look at the patch. I wouldn't hold my breath on it making upstream. It's quite tailored to the Redis/Valkey use case IMO. For instance, it focuses on slab allocations only while the same external fragmentation could happen to larger allocations too in theory, the implementation decision to focus on smaller slab allocations is based on the Redis context. Making this API a proper/generic one that can work with a wide range of applications would not be easy; but until then I see little hope of it making upstream.

Also I am not sure how to quantify the need for such a precise defrag signal in the first place without some benchmarking work that compares this approach vs, say, a simple "move-all" approach.

We should probably look for different heuristics on the Valkey side. Maybe we could track some additional allocation stats so we have a sense of how the allocation pattern shifts. In the example of 150B vs 300B as mentioned in jemalloc discussion, if we can tell from which size to the other the allocation pattern is shifting, more (defrag) weights can then be given to the older size (be it 150B or 300B) without any new jemalloc APIs.

from valkey.

btw, we should probably track the de-vendoring of different dependents in their own issues. The reasons for vendoring and the solutions will likely be different.

from valkey.

It's also probably worth mentioning we could still support "vendored" jemalloc + "custom" jemalloc. It sounds like the original requestor might be okay with no active defrag (which I think has it's limitations) in order to by able to dynamically link with the local jemalloc.

from valkey.