Remove valkey specific changes in jemalloc source code

[zvi-code]

*note: this PR replaced prior PR

Summary of the change

This is a base PR for refactoring defrag. It moves the defrag logic to rely on jemalloc native api instead of relying on custom code changes made by valkey in the jemalloc (je_defrag_hint) library. This enables valkey to use latest vanila jemalloc without the need to maintain code changes cross jemalloc versions.

This change requires some modifications because the new api is providing only the information, not a yes\no defrag. The logic needs to be implemented at valkey code. Additionally, the api does not provide, within single call, all the information needed to make a decision, this information is available through additional api call. To reduce the calls to jemalloc, in this PR the required information is collected during the computeDefragCycles and not for every single ptr, this way we are avoiding the additional api call.
Followup work will utilize the new options that are now open and will further improve the defrag decision and process.

Added files:

allocator_defrag.c / allocator_defrag.h - This files implement the allocator specific knowledge for making defrag decision. The knowledge about slabs and allocation logic and so on, all goes into this file. This improves the separation between jemalloc specific code and other possible implementation.

Moved functions:

zmalloc_no_tcache , zfree_no_tcache - these are very jemalloc specific logic assumptions, and are very specific to how we defrag with jemalloc. This is also with the vision that from performance perspective we should consider using tcache, we only need to make sure we don't recycle entries without going through the arena [for example: we can use private tcache, one for free and one for alloc].
frag_smallbins_bytes - the logic and implementation moved to the new file

Existing API:

• [once a second + when completed full cycle] computeDefragCycles
  • zmalloc_get_allocator_info : gets from jemalloc allocated, active, resident, retained, muzzy, frag_smallbins_bytes
  • frag_smallbins_bytes : for each bin; gets from jemalloc bin_info, curr_regs, cur_slabs
• [during defrag, for each pointer]
  • je_defrag_hint is getting a memory pointer and returns {0,1} . Internally it uses this information points:
    • #nonfull_slabs
    • #total_slabs
    • #free regs in the ptr slab

Jemalloc API (via ctl interface)

[BATCH]experimental_utilization_batch_query_ctl : gets an array of pointers, returns for each pointer 3 values,

• number of free regions in the extent
• number of regions in the extent
• size of the extent in terms of bytes

[EXTENDED]experimental_utilization_query_ctl :

• memory address of the extent a potential reallocation would go into
• number of free regions in the extent
• number of regions in the extent
• size of the extent in terms of bytes
• [stats-enabled]total number of free regions in the bin the extent belongs to
• [stats-enabled]total number of regions in the bin the extent belongs to

experimental_utilization_batch_query_ctl vs valkey je_defrag_hint?

[good]

• We can query pointers in a batch, reduce the overall overhead
• The per ptr decision algorithm is not within jemalloc api, jemalloc only provides information, valkey can tune\configure\optimize easily

[bad]

• In the batch API we only know the utilization of the slab (of that memory ptr), we don’t get the data about #nonfull_slabs and total allocated regs.

New functions:

1. defrag_jemalloc_init: Reducing the cost of call to je_ctl: use the MIB interface to get a faster calls. See this quote from the jemalloc documentation:

   The mallctlnametomib() function provides a way to avoid repeated name lookups for
   applications that repeatedly query the same portion of the namespace,by translating
   a name to a “Management Information Base” (MIB) that can be passed repeatedly to
   mallctlbymib().

2. jemalloc_sz2binind_lgq* : this api is to support reverse map between bin size and it’s info without lookup. This mapping depends on the number of size classes we have that are derived from lg_quantum

3. defrag_jemalloc_get_frag_smallbins : This function replaces frag_smallbins_bytes the logic moved to the new file allocator_defrag
   defrag_jemalloc_should_defrag_multi → handle_results - unpacks the results

4. should_defrag : implements the same logic as the existing implementation inside je_defrag_hint

5. defrag_jemalloc_should_defrag_multi : implements the hint for an array of pointers, utilizing the new batch api. currently only 1 pointer is passed.

Logical differences:

In order to get the information about #nonfull_slabs and #regs, we use the query cycle to collect the information per size class. In order to find the index of bin information given bin size, in o(1), we use jemalloc_sz2binind_lgq* .

Testing

This is the first draft. I did some initial testing that basically fragmentation by reducing max memory and than waiting for defrag to reach desired level. The test only serves as sanity that defrag is succeeding eventually, no data provided here regarding efficiency and performance.

Test:

1. disable activedefrag
2. run valkey benchmark on overlapping address ranges with different block sizes
3. wait untill used_memory reaches 10GB
4. set maxmemory to 5GB and maxmemory-policy to allkeys-lru
5. stop load
6. wait for mem_fragmentation_ratio to reach 2
7. enable activedefrag - start test timer
8. wait until reach mem_fragmentation_ratio = 1.1

Results*:

(With this PR)Test results: 56 sec
(Without this PR)Test results: 67 sec

*both runs perform same "work" number of buffers moved to reach fragmentation target

Next benchmarking is to compare to:

• DONE // existing je_get_defrag_hint
• compare with naive defrag all: int defrag_hint() {return 1;}

[zvi-code]

This PR is replacing https://github.com/valkey-io/valkey/pull/692.

It consists of 2 commits:

1. A cherry-pick of the original commit from PR 692 (on top of unstable)
2. A CR fixes commit to address the comments made on PR 692

[codecov]

Codecov Report

Attention: Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.73%. Comparing base (2df56d8) to head (2a6c2df).
Report is 13 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/server.c	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1266      +/-   ##
============================================
+ Coverage     70.69%   70.73%   +0.04%     
============================================
  Files           114      116       +2     
  Lines         63161    63239      +78     
============================================
+ Hits          44650    44732      +82     
+ Misses        18511    18507       -4     

Files with missing lines	Coverage Δ
src/allocator_defrag.c	100.00% <100.00%> (ø)
src/defrag.c	84.68% <100.00%> (-1.58%)	⬇️
src/server.h	100.00% <ø> (ø)
src/zmalloc.c	82.60% <100.00%> (-2.07%)	⬇️
src/server.c	87.64% <50.00%> (-0.06%)	⬇️

... and 20 files with indirect coverage changes

[madolson]

Mostly minor comments, and all the tests are still passing which is good. I might take another pass after this to just cleanup some comments for clarity.

[zvi-code]

@madolson, I fixed the comments

[madolson]

It LGTM, only sticky point left is just some follows ups on the info fields. Would appreciate if one of @JimB123 or @zuiderkwast have time to take a look as well to review the jemalloc logic.

[zvi-code]

@valkey-io/core-team this PR replaces previous PR due to git history issues i could not fix. Can we put the 8.1 target (and if it's important than also mark with major decision approved)

[zuiderkwast]

this PR replaces #692 due to git history issues i could not fix

@zvi-code OK. It's possible to completely replace the content of a PR by force-pushing completely new commits to the same branch name though. Next time. :)

[zuiderkwast]

Yes! This looks very close merging now. Just some nits and some questions.

[JimB123]

I like that we are eliminating a custom version of jemalloc. If we can upgrade jemalloc as a drop-in replacement, that's a win.

[JimB123]

If what I said above is correct, this doesn't make sense to me. Are we computing the total number of regions in the BIN less the number of free regions in the SLAB? I don't understand what that would represent, or why. Is this a bug? or do we need more comments here.

[zvi-code]

No, please see documentation:

/* Represents detailed information about a jemalloc bin.
 *
 * This struct provides metadata about a jemalloc bin, including the size of
 * its regions, total number of regions, and related MIB keys for efficient
 * queries.
 */
typedef struct jeBinInfo {
    unsigned long reg_size;  /* Size of each region in the bin. */
    unsigned long nregs;     /* Total number of regions in the bin. */
    jeBinInfoKeys info_keys; /* Precomputed MIB keys for querying bin statistics. */
} jeBinInfo;

arena_bin_conf.bin_info holds metadata for each bin id (binind), as sucharena_bin_conf.bin_info[binind].nregs is the number of regions in each slab in this bin, it does not change after initial information is retrieved during init.

[JimB123]

The code above says that nregs is the "total number of regions in the bin". You just said that it's the number of regions per slab. Which is it?

[zvi-code]

The hole struct holds static once initialized information about the bin. As such obviously it's not the number of regs in the bin at specific moment, but how many regs are there in a slab of this bin. I can change the comment to clarify if it's not clear

[zvi-code]

@madolson , @JimB123 , @zuiderkwast , updated PR with fixes to the comments, please review

[madolson]

Official, stamp, will wait to see if others want to take another review pass before merging.

[madolson]

I was just going to add that we have a few of the zmalloc functions can now optionally take the size to improve the performance. See

valkey/src/zmalloc.c

Line 439 in aa2dd3e

void zfree_with_size(void *ptr, size_t size) {

for an example.

[zuiderkwast]

Looks good in general. I haven't reviewed all the details. I just have a few nits and some questions.

What exactly is the relationship between the units zmalloc and allocator_defrag? Both of them are allocator abstractions, but one doesn't depend on the other. They're on the same level, but they still have some dependencies to each other: Both have to be using the same allocator.

This structure seems fine to me. Perhaps we could add some comment in each of them to refer to the other file or something.

        Application code
             /    \
 allocation /      \ defrag
           /        \
      zmalloc       allocator_defrag
      /  |   \       /     \
     /   |    \     /       \
    /    |     \   /         \
libc  tcmalloc  jemalloc     other

[zvi-code]

Looks good in general. I haven't reviewed all the details. I just have a few nits and some questions.

What exactly is the relationship between the units zmalloc and allocator_defrag? Both of them are allocator abstractions, but one doesn't depend on the other. They're on the same level, but they still have some dependencies to each other: Both have to be using the same allocator.

This structure seems fine to me. Perhaps we could add some comment in each of them to refer to the other file or something.

        Application code
             /    \
 allocation /      \ defrag
           /        \
      zmalloc       allocator_defrag
      /  |   \       /     \
     /   |    \     /       \
    /    |     \   /         \
libc  tcmalloc  jemalloc     other

@zuiderkwast , I like your diagram, it is correct. We could have a non allocator defragmentation logic, for example think about lazy listpack\rax compaction, but i removed this abstraction to reduce the scope of the PR. How about this documentation along with the diagram? [I got some help]

/*
 * This file implements allocator-specific defragmentation logic used
 * within the Valkey engine. Below is the relationship between various
 * components involved in allocation and defragmentation:
 *
 *                  Application code
 *                     /       \
 *         allocation /         \ defrag
 *                   /           \
 *              zmalloc    allocator_defrag
 *               /  |   \       /     \
 *              /   |    \     /       \
 *             /    |     \   /         \
 *        libc  tcmalloc  jemalloc     other
 *
 * Explanation:
 * - **Application code**: High-level application logic that uses memory
 *   allocation and may trigger defragmentation.
 * - **zmalloc**: An abstraction layer over the memory allocator, providing
 *   a uniform allocation interface to the application code. It can delegate
 *   to various underlying allocators (e.g., libc, tcmalloc, jemalloc, or others).
 *   It is not dependant on defrag implementation logic and it's possible to use jemalloc
 *   version that does not support defrag.
 * - **allocator_defrag**: This file contains allocator-specific logic for
 *   defragmentation, invoked from `defrag.c` when memory defragmentation is needed.
 *   currently jemalloc is the only allocator with implemented defrag logic. It is possible that 
 *   future implementation will include non-allocator defragmentation (think of data-structure
 *   compaction for example). 
 * - **Underlying allocators**: These are the actual memory allocators, such as
 *   libc, tcmalloc, jemalloc, or other custom allocators. The defragmentation
 *   logic in `allocator_defrag` interacts with these allocators to reorganize
 *   memory and reduce fragmentation.
 *
 * The `defrag.c` file acts as the central entry point for defragmentation,
 * invoking allocator-specific implementations provided here in `allocator_defrag.c`.
 *
 * Note: Developers working on `zmalloc` or `allocator_defrag` should refer to
 * the other component to ensure both are using the same allocator configuration.
 */

[zuiderkwast]

LGTM

[JimB123]

I don't think you understood my previous comment. In the first call, on 267, you return -1 if it fails. On the 2nd call, you assert. Why are there 2 different patterns for the same function call?

[zvi-code]

The first case, could fail and does not o dictate bug by itself, because the jemalloc version does not have the query api, from this code perspective it's a valid case and -1 is returned to indicate that init didn't succeed. The following calls are expected to succeed and are asserted to verify. I moved the call out of the assert following your comment I believe.