I love this, thanks @oranagra, do you have information about its safeness? Are we likely to trigger some jemalloc bug given that we have a non standard size class with this patch, or it is a widespread feature? Thanks.

we've been using this patch for quite some time now, and it seems safe.

I consulted the jemalloc mailing list about it, and they suggested a different approach (with command line arguments that changes LG_QUANTUM), but i like mine better especially since it fixes what seems to be a minor bug in that script, and i don't think changing the platform quantum is the right thing to do (i rather add more tiny classes instead).

bottom line is that if you look at the code this script generates, it just adds one more bin size to a table that the code works with. i don't see any risk in that, and a lot of testing over time already proved it works good.

Thanks for the clarifications, not enough background to follow your reasoning, no idea what jemalloc LG_QUANTUM is, but I'll check for sure. It's awesome that you already have this in production and works well, I'll merge into unstable soon after some review, that in that case, will be just to make my mind a bit about the jemalloc mailing list discussion. I'll comment more after the review. It's pretty important we get this merged before I merge unstable into testing, so that we can have this as part of Redis 3.2. The same for the other memory related pull request. I guess that this and the other together may account for some quite impressive memory savings!

⭐

Hello again @oranagra, I did my homework studying the Jemalloc script and understanding the LG_QUANTUM macro.

I'm a bit biased towards the solution of changing LG_QUANTUM to 3, instead of the script, for a couple of reasons:

1. LG_QUANTUM is a smaller change and simpler to manage when we upgrade jemalloc, especially since upcoming versions of jemalloc will have --with-lg-quantum configure option, so we'll be able to take, like today, the unmodified version of jemalloc inside deps.
2. The additional size classes look interesting. Changing the LG_QUANTUM means that we also get a 40 size class that I see very relevant to the Redis use case.
3. It is simple to enable it just for Linux, which may be the safe thing to do, given that glibc malloc already returns non 16 bytes aligned pointers, so Linux is tested to cope with that, but maybe other archs are not.

That said apparently the 16 bytes alignment is only used for SSE, and otherwise the larger C type is 8 bytes so we are unlikely to have problems anyway.

That said, I guess that the following change should do for now?

diff --git a/deps/jemalloc/include/jemalloc/internal/jemalloc_internal.h.in b/dep
index 574bbb1..df266ab 100644
--- a/deps/jemalloc/include/jemalloc/internal/jemalloc_internal.h.in
+++ b/deps/jemalloc/include/jemalloc/internal/jemalloc_internal.h.in
@@ -242,7 +242,7 @@ static const bool config_ivsalloc =
  */
 #ifndef LG_QUANTUM
 #  if (defined(__i386__) || defined(_M_IX86))
-#    define LG_QUANTUM         4
+#    define LG_QUANTUM         3
 #  endif
 #  ifdef __ia64__
 #    define LG_QUANTUM         4
@@ -254,7 +254,7 @@ static const bool config_ivsalloc =
 #    define LG_QUANTUM         4
 #  endif
 #  if (defined(__amd64__) || defined(__x86_64__) || defined(_M_X64))
-#    define LG_QUANTUM         4
+#    define LG_QUANTUM         3
 #  endif
 #  ifdef __arm__
 #    define LG_QUANTUM         3

I would just add a few ifdefs in order to make sure only Linux get this change. I'm going to test in a few data sets the difference it makes in practice. Feedbacks welcomed.

p.s. actually long double is 16 bytes and may also create issues but AFAIK we don't use it inside Redis. Moreover to create problems it should be allocated in the context of a size class which is not multiple of 16.

Hi,
As long as we get 24 byte pool, i don't really care if its this patch or the other..
The reasons i used my own changes to the scripts are these (assuming my memory doesn't fail me.. this was a long time ago):

1. If i understand correctly, LG_QUANTUM means the normal memory alignment for the platform. it seemed wrong to change that (when all i really want to do is add more resolution to the area that is using the tiny classes).
   It seems to add more tiny classes as the expense of Quantum-multiple size classes. so, many of the classes that were quantum sizes, will now in the tiny category (i wasn't sure if that has any side effects or not).

If we consider the jemalloc mailinglist reply trustworthy, then i guess it doesn't have any memory alignment side effects. but i didn't test it myself.

1. i asked the jemalloc mailing list and didn't get an answer for a long time.. when i finally got an answer it was after i already used my fix and moved on...
2. the change i did in line 55 of the script is actually a bug fix, which didn't affect them until i did my change in line 51... so my change is actually just the one in line 51 (and the updated comment above it)

Regarding your concerns about alignment.
If a 24 byte struct would have had any member which is 8 byte native type, the compiler would have naturally added padding (so that it can be used in an array), and then it would no longer be 24 bytes. so that's perfectly safe.

My view regarding applying it only for Linux is that we shouldn't do that out of fear.
If we do, it means that other platforms just loose some optimizations and we'll never really know if that's a required sacrifice or just an undesired loss due to the platform not being the developer's main platform.
Unless i know there's a problem in specific platform, i would have enabled it for all and wait to get feedback. and then revert only in places that problems were found.

anyway, my bottom line is that i don't really care this way or the other.. these were my 2 cents....
the only important part is that we get the 24 byte class (which is used by dictEntry and listNode and others).
few minutes of work, few lines of change, huge impact on memory footprint.

Hello Oran,

Yep LG_QUANTUM affects more than just the size class, however if you add a 24 size class you are effectively just violated LG_QUANTUM, since jemalloc now returns non 16 bytes aligned stuff for certain allocations. So the risky part is equivalent (but read later about the risk). About moving more classes into tiny compared to quantum, I don't think this is going to have side effects since there are already platforms using LG_QUANTUM set to 3 (apparently ARM is one of those if I read it correctly in emalloc/include/jemalloc/internal/jemalloc_internal.h.in.

I agree about the fact your change fixes the script, but IMHO this should be sent as a pull request to jemalloc for them to fix this, instead of patching this ourselves. I believe it is better if we just depend on the things are standard and tested for jemalloc (even if broken! Like the script). We know there are people running with LG_QUANTUM set to 3 and they are even going to add it to configure... So apparently for now that's the way to go.

Moreover I really believe that if we go for more fine grained size classes, there is no reason to don't get the 40 size class, and the non 16 bytes aligned others.

About the safety of all this, as you said unless a structure is not packed we are safe, moreover... we are using our internal jemalloc only for the goal of Redis. This is very different than targeting a system wide jemalloc, so probably we are completely safe both on Linux and other OSes, since even if Linux has workarounds for the SEE case or others, we just don't care, we don't use any 16 bytes aligned data type. So I agree with you that after all there is no reason to protect with Linux specific defines... Without to mention that Jemalloc itself is compiled by default only on Linux.

I'm making the change and committing in a a few. Update ASAP. Thanks!

What about that?

Unstable original memory usage (DEBUG POPULATE 1000000): 115.57M, after SDS changes 92.69M, after jemalloc new classes: 77.43M

💃

All tests passing. We can close this as well, thanks @oranagra!

@antirez How about allocating performance? saving memory is amazing :)