Rax size tracking #688

knggk · 2024-06-24T21:35:43Z

Introduce a size_t field into the rax struct to track allocation size.
Update the allocation size on rax insert and deletes.
Return the allocation size when raxAllocSize is called.

This size tracking is now used in MEMORY USAGE and MEMORY STATS in place of the previous method based on sampling.

The module API allows to create sorted dictionaries, which are backed by rax. Users now also get precise memory allocation for them (through ValkeyModule_MallocSizeDict).

Fixes #677.

For the release notes:

MEMORY USAGE and MEMORY STATS are now exact for streams, rather than based on sampling.

kyle-yh-kim · 2024-06-25T17:38:42Z

To resolve DCO complaints, you can sign-off the previous commits.

// Sign-off the last 3 commits.
git rebase --signoff HEAD~3

// Force push to update your PR.
git push -f

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

kyle-yh-kim · 2024-06-25T18:53:35Z

Discussed with @knggk offline. A few parallelizable work for this PR;

Import rax-test.c and stabilize it [source].
Devise new test cases under rax-test.c, for size tracking.
MEMORY STATS integration.

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

codecov · 2024-06-26T22:46:01Z

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.60%. Comparing base (9827eef) to head (fee849d).
Report is 1 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/object.c	0.00%	3 Missing ⚠️
src/module.c	0.00%	2 Missing ⚠️
src/rax.c	91.30%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #688      +/-   ##
============================================
+ Coverage     70.47%   70.60%   +0.13%     
============================================
  Files           114      114              
  Lines         61695    61710      +15     
============================================
+ Hits          43479    43572      +93     
+ Misses        18216    18138      -78

Files with missing lines	Coverage Δ
src/module.c	`9.64% <0.00%> (+<0.01%)`	⬆️
src/rax.c	`82.92% <91.30%> (+0.22%)`	⬆️
src/object.c	`79.18% <0.00%> (+0.53%)`	⬆️

... and 13 files with indirect coverage changes

zuiderkwast

In general LGTM. The test needs to be ported to the new kind of unit tests.

Is is possible to test the calculation of the alloc size by comparing it to the stats we can get from the allocator? If we run a fuzz test and it does no other allocations, then it could work I think.

src/rax.h

src/server.c

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk · 2024-06-27T21:42:22Z

In general LGTM. The test needs to be ported to the new kind of unit tests.

Is is possible to test the calculation of the alloc size by comparing it to the stats we can get from the allocator? If we run a fuzz test and it does no other allocations, then it could work I think.

Great Idea. I wanted also to check that after every element from the rax was removed, memory goes back to 0. Turns out there is an issue right now that needs further investigation, ie src/valkey-unit-tests --large-memory says:

...
[ok] - test_rax.c:test_hugeKey
...
[END] - test_rax.c: 12 tests, 12 passed, 0 failed
...
[START] - test_zmalloc.c
[test_zmallocInitialUsedMemory - unit/test_zmalloc.c:9] Failed assertion: zmalloc_used_memory() == 0

zuiderkwast · 2024-06-27T23:49:17Z

Just an idea: If it's a problem with zmalloc, you can create another rax_test_malloc.h to instead of rax_malloc.h just for the rax test. If the test defines RAX_MALLOC_INCLUDE before it includes rax.c, then rax.c will use it. It's already like this in rax.c:

#ifndef RAX_MALLOC_INCLUDE
#define RAX_MALLOC_INCLUDE "rax_malloc.h"
#endif

#include RAX_MALLOC_INCLUDE

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk · 2024-06-28T21:03:15Z

RAX_MALLOC_INCLUDE

Thanks for the tip @zuiderkwast . I think it wasn't a problem with zmalloc, see the latest two commits. There was a zfree missing which was the cause of valkey-unit-tests --large-memory failing.

However you gave me another idea: what if instead of manually tracking rax->alloc_size in the main code body, we made rax_alloc_size and friends do it, as in rax_alloc_size(rax, size) { void *x = zmalloc(size); rax->alloc_size += zmalloc_usable_memory(x); x } ? It sounds like it'd be harder to make mistakes like the one fixed in "Tentative fix for address sanitizer", which tried to use an address that's already been freed. What do you think?

src/rax.c

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

zuiderkwast · 2024-07-09T19:04:02Z

Please check clang-format and spellcheck. You can change "ba" to "by" (etc.) in the rax_test, or if this actually affects the test, it's OK to add this file to .config/typos.toml to exclude the file from spell check.

knggk · 2024-07-17T15:58:16Z

I see. I had a quick exchange with @touitou-dan who was suggesting a 2% degradation in pipeline only can be ignored. After all pipeline is not the standard mode of benchmark.

In light of that, do we know the degradation of the memory tracking for the other pieces? If similar, maybe it's worth not making it configurable. Thoughts @zuiderkwast?

zuiderkwast · 2024-07-17T16:12:03Z

This will have to wait until ~~8.2~~ 8.1, so we should think about it after 8.0 rc1...

ranshid · 2024-09-29T12:44:44Z

@knggk Did we consider ways to avoid calling zmalloc_size on each rax operation? IIRC these can be expensive and we should aim to avoid them. For example It might be messy but we could try and use the jemalloc *_usable API when allocating memory and retrieve the allocation size.
Also as we now track the used_memory_thread it might be easier to just take the diff during RAX operations instead of going through the "expensive" zmalloc_size calls?

knggk · 2024-09-30T19:28:23Z

Did we consider ways to avoid calling zmalloc_size on each rax operation?

But it only happens on rax mutations right?

try and use the jemalloc *_usable API when allocating memory and retrieve the allocation size.

What are those? Are you saying they are less expensive than zmalloc_size? What's the context behind zmalloc_size being expensive?

Also as we now track the used_memory_thread it might be easier to just take the diff during RAX operations instead of going through the "expensive" zmalloc_size calls?

Agree, good idea if this new used_memory_thread is updated during allocation calls and would capture a diff when called right before allocating eg a raxNode, and again right after. Is this how used_memory_thread behaves?

ranshid · 2024-10-01T15:04:54Z

Did we consider ways to avoid calling zmalloc_size on each rax operation?

But it only happens on rax mutations right?

yes

try and use the jemalloc *_usable API when allocating memory and retrieve the allocation size.

What are those? Are you saying they are less expensive than zmalloc_size? What's the context behind zmalloc_size being expensive?

I mean for example there is the 'oid *zmalloc_usable(size_t size, size_t *usable)' which both allocate memory and return the allocation size, so it can be used in a single call instead of later calling 'zmalloc_size'

Also as we now track the used_memory_thread it might be easier to just take the diff during RAX operations instead of going through the "expensive" zmalloc_size calls?

Agree, good idea if this new used_memory_thread is updated during allocation calls and would capture a diff when called right before allocating eg a raxNode, and again right after. Is this how used_memory_thread behaves?

Basically since we are taking the diff without being preempted and doing "something else" I think we can count of this diff to reflect the exact amount of memory we allocated/freed during the rax operation.

zuiderkwast · 2024-10-01T15:23:33Z

I mean for example there is the void *zmalloc_usable(size_t size, size_t *usable) which both allocate memory and return the allocation size, so it can be used in a single call instead of later calling 'zmalloc_size'

Good idea. If it's not too complicated, it would be better to use it, but I'm not sure zmalloc_size is very slow either. Maybe it's not a significant part of the work in the rax update.

Basically since we are taking the diff without being preempted and doing "something else" I think we can count of this diff to reflect the exact amount of memory we allocated/freed during the rax operation.

I think this can work, but note that used_memory_thread per thread is a size_t which can wrap around zero. For example, if one thread T1 allocates some memory and another thread T2 frees the memory, then T2 can end up freeing more memory than it allocates, i.e. negative number, wrapping from 0 to SIZE_MAX. It can probably be handled. Just be careful.

IMO, we can also accept this PR with zmalloc_size now and allow it to be optimized in the future.

ranshid · 2024-10-01T16:27:54Z

IMO, we can also accept this PR with zmalloc_size now and allow it to be optimized in the future.

Can we open an issue for that then?

ranshid · 2024-10-01T17:55:43Z

I think this can work, but note that used_memory_thread per thread is a size_t which can wrap around zero. For example, if one thread T1 allocates some memory and another thread T2 frees the memory, then T2 can end up freeing more memory than it allocates, i.e. negative number, wrapping from 0 to SIZE_MAX. It can probably be handled. Just be careful.

Not sure there is a problem. we only take a diff of the thread local variable, so even if it was negative the diff calculation will still be fine.

zuiderkwast · 2024-10-01T18:30:58Z

I think this can work, but note that used_memory_thread per thread is a size_t which can wrap around zero. For example, if one thread T1 allocates some memory and another thread T2 frees the memory, then T2 can end up freeing more memory than it allocates, i.e. negative number, wrapping from 0 to SIZE_MAX. It can probably be handled. Just be careful.

Not sure there is a problem. we only take a diff of the thread local variable, so even if it was negative the diff calculation will still be fine.

@ranshid 🤔 You're probably right.

Another problem is that we only track a certain number of threads. If a module creates a lot of threads and they use the rax (e.g. using ValkeyModule_DictXxxx functions) these threads will not have their own variable.

I don't know how to solve that. Do you have an idea? The per-thread size tracking was not designed for what we're discussing here. It was only designed to speed up the total size tracking. Letting the rax track its own allocations seems more robust to me somehow.

Can we open an issue for that then?

Sure.

@knggk, will you merge latest unstable to this branch?

knggk · 2024-10-01T20:34:53Z

@knggk, will you merge latest unstable to this branch?

I tried just now but am getting:

% make valkey-unit-tests
...
    LINK valkey-unit-tests
duplicate symbol '__serverAssert' in:
    /Users/knggk/valkey/src/unit/test_main.o
    libvalkey.a[36](debug.o)
duplicate symbol '_main' in:
    /Users/knggk/valkey/src/unit/test_main.o
    libvalkey.a[9](server.o)
ld: 2 duplicate symbols
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Edit: Nvm, this also happens on clean unstable. Might be a problem with my local toolchain?

zuiderkwast · 2024-10-02T10:08:51Z

Edit: Nvm, this also happens on clean unstable. Might be a problem with my local toolchain?

Seems to be, because the build works in the CI. Did you try make distclean?

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk · 2024-10-02T16:18:58Z

Seems to be, because the build works in the CI. Did you try make distclean?

I tried with distclean but still getting the same issue on link. I am on Mac Sonoma if that's any useful info. Could is be because of missing pkg-config? I get:

% make valkey-unit-tests
make valkey-unit-tests
cd src && /Library/Developer/CommandLineTools/usr/bin/make valkey-unit-tests
/bin/sh: pkg-config: command not found
/bin/sh: pkg-config: command not found
/bin/sh: pkg-config: command not found
...

PS pushed commit to fix formatting

zuiderkwast · 2024-10-02T16:27:48Z

Great, thanks.

Could is be because of missing pkg-config? I get:

Possibly. I'm running Fedora on my macbook. It seemed better for programmers. :D

pkg-config is used in the Makefile and maybe it just exits if it's not available. Why don't you have it? And why don't you have make in your path? Maybe you just need some basic "build util" package from brew or something...?

zuiderkwast · 2024-10-03T07:42:39Z

@knggk There was a build failure with undefined-santitizer in Daily:

...
Cluster Fuzz test [keys:99963652 keylen:0]: ok with 71243660 final keys
Cluster Fuzz test [keys:37437816 keylen:0]: ok with 34387942 final keys
Cluster Fuzz test [keys:22357214 keylen:0]: ok with 5758528 final keys
Fuzz test in mode 0 [9067]: 8396 elements inserted
unit/test_rax.c:168:15: runtime error: left shift of 36625 by 16 places cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior unit/test_rax.c:168:15 in 
Fuzz test in mode 1 [7504]:

~~Do you want to look into it?~~ You need to build with SANITIZER=undefined and run the tests with --accurate.

I have a fix for it in #1122.

knggk · 2024-10-03T14:02:51Z

Thanks Viktor for the turn around, I hadn't seen the issue.

Fix the warning introduced in #688: ``` unit/test_rax.c:168:15: runtime error: left shift of 36625 by 16 places cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior unit/test_rax.c:168:15 in Fuzz test in mode 1 [7504]: ``` Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Introduce a `size_t` field into the rax struct to track allocation size. Update the allocation size on rax insert and deletes. Return the allocation size when `raxAllocSize` is called. This size tracking is now used in MEMORY USAGE and MEMORY STATS in place of the previous method based on sampling. The module API allows to create sorted dictionaries, which are backed by rax. Users now also get precise memory allocation for them (through `ValkeyModule_MallocSizeDict`). Fixes valkey-io#677. For the release notes: * MEMORY USAGE and MEMORY STATS are now exact for streams, rather than based on sampling. --------- Signed-off-by: Guillaume Koenig <knggk@amazon.com> Signed-off-by: Guillaume Koenig <106696198+knggk@users.noreply.github.com> Co-authored-by: Joey <yzhaon@amazon.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: naglera <anagler123@gmail.com>

Fix the warning introduced in valkey-io#688: ``` unit/test_rax.c:168:15: runtime error: left shift of 36625 by 16 places cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior unit/test_rax.c:168:15 in Fuzz test in mode 1 [7504]: ``` Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: naglera <anagler123@gmail.com>

Introduce a `size_t` field into the rax struct to track allocation size. Update the allocation size on rax insert and deletes. Return the allocation size when `raxAllocSize` is called. This size tracking is now used in MEMORY USAGE and MEMORY STATS in place of the previous method based on sampling. The module API allows to create sorted dictionaries, which are backed by rax. Users now also get precise memory allocation for them (through `ValkeyModule_MallocSizeDict`). Fixes valkey-io#677. For the release notes: * MEMORY USAGE and MEMORY STATS are now exact for streams, rather than based on sampling. --------- Signed-off-by: Guillaume Koenig <knggk@amazon.com> Signed-off-by: Guillaume Koenig <106696198+knggk@users.noreply.github.com> Co-authored-by: Joey <yzhaon@amazon.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fix the warning introduced in valkey-io#688: ``` unit/test_rax.c:168:15: runtime error: left shift of 36625 by 16 places cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior unit/test_rax.c:168:15 in Fuzz test in mode 1 [7504]: ``` Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Introduce a `size_t` field into the rax struct to track allocation size. Update the allocation size on rax insert and deletes. Return the allocation size when `raxAllocSize` is called. This size tracking is now used in MEMORY USAGE and MEMORY STATS in place of the previous method based on sampling. The module API allows to create sorted dictionaries, which are backed by rax. Users now also get precise memory allocation for them (through `ValkeyModule_MallocSizeDict`). Fixes valkey-io#677. For the release notes: * MEMORY USAGE and MEMORY STATS are now exact for streams, rather than based on sampling. --------- Signed-off-by: Guillaume Koenig <knggk@amazon.com> Signed-off-by: Guillaume Koenig <106696198+knggk@users.noreply.github.com> Co-authored-by: Joey <yzhaon@amazon.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Fix the warning introduced in valkey-io#688: ``` unit/test_rax.c:168:15: runtime error: left shift of 36625 by 16 places cannot be represented in type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior unit/test_rax.c:168:15 in Fuzz test in mode 1 [7504]: ``` Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

yzhaon and others added 3 commits June 25, 2024 18:31

Track rax allocation size in rax header

12578aa

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

fix use after free, don't update the alloc field in raxFree

aaebb31

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Fix issues with previous attempt

e09bef3

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk force-pushed the rax-size-tracking branch from 3881192 to e09bef3 Compare June 25, 2024 21:13

yzhaon and others added 6 commits June 25, 2024 21:16

Add rax-test from antirez repo into Redis repo

1acaa41

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

replace rc4rand with twin prime generator

9ac6f88

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Replace usage of libc alloc fn calls with zmalloc calls

2de1d2e

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Remove time function, remove argv parsing code

4909a1f

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Add features of the unit test as flags

eeb0f2e

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Fix rax-test so that it compiles and tests pass

88cbb16

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk mentioned this pull request Jun 26, 2024

Introduce new rax function raxAllocSize to return rax tree allocation size in constant time #677

Closed

zuiderkwast reviewed Jun 26, 2024

View reviewed changes

src/rax.h Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

knggk added 3 commits June 27, 2024 17:34

Move rax-test.c under src/unit/test_rax.c

f66ba88

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Adapt rax tests to the new unit test framework

77f1b07

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

s/rax->alloc/rax->alloc_size/

6cf1d96

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk added 2 commits June 28, 2024 16:14

Tentative fix for address sanitizer

11a5a4f

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Fix missing zfree for src/valkey-unit-tests --large-memory

da095ac

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk commented Jun 28, 2024

View reviewed changes

src/rax.c Show resolved Hide resolved

zuiderkwast reviewed Jul 3, 2024

View reviewed changes

src/rax.c Outdated Show resolved Hide resolved

src/rax.c Outdated Show resolved Hide resolved

Reduce scope of variable

581fdbe

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk force-pushed the rax-size-tracking branch from 237e3f9 to 581fdbe Compare July 3, 2024 21:56

knggk added 2 commits July 5, 2024 18:34

s/rax_alloc_size/rax_ptr_alloc_size/

a254c28

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Demo checking rax size vs allocator

59169c0

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

Merge remote-tracking branch 'origin/unstable' into rax-size-tracking

eed12f2

Merge remote-tracking branch 'origin/unstable' into rax-size-tracking

6b72c68

Fix formatting

fee849d

Signed-off-by: Guillaume Koenig <knggk@amazon.com>

knggk force-pushed the rax-size-tracking branch from 28951d3 to fee849d Compare October 2, 2024 16:14

zuiderkwast added the release-notes This issue should get a line item in the release notes label Oct 2, 2024

zuiderkwast merged commit f85d8bf into valkey-io:unstable Oct 2, 2024
47 checks passed

zuiderkwast mentioned this pull request Oct 2, 2024

Optimize rax size tracking #1109

Open

zuiderkwast mentioned this pull request Oct 3, 2024

Fix undefined-santitizer warning in rax test #1122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rax size tracking #688

Rax size tracking #688

knggk commented Jun 24, 2024 •

edited by zuiderkwast

Loading

kyle-yh-kim commented Jun 25, 2024

kyle-yh-kim commented Jun 25, 2024

codecov bot commented Jun 26, 2024 •

edited

Loading

zuiderkwast left a comment

knggk commented Jun 27, 2024

zuiderkwast commented Jun 27, 2024 •

edited

Loading

knggk commented Jun 28, 2024

zuiderkwast commented Jul 9, 2024

knggk commented Jul 17, 2024 •

edited

Loading

zuiderkwast commented Jul 17, 2024 •

edited

Loading

ranshid commented Sep 29, 2024

knggk commented Sep 30, 2024

ranshid commented Oct 1, 2024

zuiderkwast commented Oct 1, 2024

ranshid commented Oct 1, 2024

ranshid commented Oct 1, 2024

zuiderkwast commented Oct 1, 2024

knggk commented Oct 1, 2024 •

edited

Loading

zuiderkwast commented Oct 2, 2024

knggk commented Oct 2, 2024

zuiderkwast commented Oct 2, 2024

zuiderkwast commented Oct 3, 2024 •

edited

Loading

knggk commented Oct 3, 2024

Rax size tracking #688

Rax size tracking #688

Conversation

knggk commented Jun 24, 2024 • edited by zuiderkwast Loading

kyle-yh-kim commented Jun 25, 2024

kyle-yh-kim commented Jun 25, 2024

codecov bot commented Jun 26, 2024 • edited Loading

Codecov Report

zuiderkwast left a comment

Choose a reason for hiding this comment

knggk commented Jun 27, 2024

zuiderkwast commented Jun 27, 2024 • edited Loading

knggk commented Jun 28, 2024

zuiderkwast commented Jul 9, 2024

knggk commented Jul 17, 2024 • edited Loading

zuiderkwast commented Jul 17, 2024 • edited Loading

ranshid commented Sep 29, 2024

knggk commented Sep 30, 2024

ranshid commented Oct 1, 2024

zuiderkwast commented Oct 1, 2024

ranshid commented Oct 1, 2024

ranshid commented Oct 1, 2024

zuiderkwast commented Oct 1, 2024

knggk commented Oct 1, 2024 • edited Loading

zuiderkwast commented Oct 2, 2024

knggk commented Oct 2, 2024

zuiderkwast commented Oct 2, 2024

zuiderkwast commented Oct 3, 2024 • edited Loading

knggk commented Oct 3, 2024

knggk commented Jun 24, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Jun 26, 2024 •

edited

Loading

zuiderkwast commented Jun 27, 2024 •

edited

Loading

knggk commented Jul 17, 2024 •

edited

Loading

zuiderkwast commented Jul 17, 2024 •

edited

Loading

knggk commented Oct 1, 2024 •

edited

Loading

zuiderkwast commented Oct 3, 2024 •

edited

Loading