-
Notifications
You must be signed in to change notification settings - Fork 899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fix zmalloc_size on macos #2646
Conversation
Added some of my fixes as well. Proof that it is passing unit tests: https://github.com/dragonflydb/dragonfly/actions/runs/8006667920 |
Signed-off-by: Roman Gershman <roman@dragonflydb.io>
182dc20
to
da337d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Andy, thank you for fixing this!
I suppose you have a mac? Can I sincerely ask you to try something for me. I would like to understand why this doesn't fail on Redis, since they use the exact same header on macos. My assumption is that Redis uses jemalloc so they actually call je_malloc_usable_size(p)
. If you do have a mac and you are happy to try out you can add an #error
to check out which path gets exercised in the header. If it does exercises the #elif defined(__APPLE__)
instead this is something that we should probably take a look at.
I pre approve but I would like this to be investigated a little more
p.s. I rerun a daily job to see the failure on the CI as well
They do not have exactly the same header. We use mi_malloc, they do not. I
changed this header to fit our requirements
…On Fri, Feb 23, 2024 at 12:12 PM Kostas Kyrimis ***@***.***> wrote:
***@***.**** approved this pull request.
Hi Andy, thank you for fixing this!
I suppose you have a mac? Can I sincerely ask you to try something for me.
I would like to understand why this doesn't fail on Redis, since they use
the exact same header on macos. My assumption is that Redis uses jemalloc
so they actually call je_malloc_usable_size(p). If you do have a mac and
you are happy to try out you can add an #error to check out which path
gets exercised in the header. If it does exercises the #elif
defined(__APPLE__) instead this is something that we should probably take
a look at.
I pre approve but I would like this to be investigated a little more
—
Reply to this email directly, view it on GitHub
<#2646 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4BFCBPZHJUAEIIRPUGWSDYVBTRRAVCNFSM6AAAAABDVBCQKCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJXG43TQMBQGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Roman Gershman
CTO
---
*www.dragonflydb.io <http://www.dragonflydb.io>*
|
I see, cheers! |
Fixes
hset_family_test
tests on MacOSLooks like
zmalloc_size
isn't defined properly on Mac, sozmalloc
usessrc/redis/zmalloc_mi.c
definition butzmalloc_size
usesmalloc_size
This just copies the
USE_ZMALLOC_MI
branch fromlibc
intomacos
branch (I don't really understand this allocator stuff but with this thehset_family_test
tests pass running locally on Mac - will run rest of the tests on mac)