Install jemalloc in docker image

[MatMaul]

It can then be used by passing -e LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so.2 to docker run.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[anoadragon453]

Is there any reason not to just make this the default behaviour?

[MatMaul]

I was being conservative, I don't see why not from my point of view. Have been using it for ages because it seems to avoid memory fragmentation and hence get back more memory to the OS.
My server being quite stable ATM regrding memory consumption, I can do a before/after on a few days of use so we can see the diff if you want.

[richvdh]

yeah, my inclination is to make it the default. Certainly a comparison would be interesting.

[MatMaul]

https://grafana.tout.im/dashboard/snapshot/YP5TNOhlPV6FTbWemQRjtLdQeP7TddZF
from 31/10 to 2/11 you can see with jemalloc, and without it afterwards.
It's not as clear cut as before.
I am planning to not reboot until at least next monday so we can see if it is still slowly fragmenting memory, it looks like it but it is really slow (~20M in the last 2 days).
I'll also do a longer jemalloc run later on.

[anoadragon453]

So it looks like jemalloc reduces memory usage, at the cost of some spikes. Does dealing with those spikes have costs associated with any other metrics, like CPU?

[MatMaul]

last round of metrics on a full week:
glibc malloc: https://grafana.tout.im/dashboard/snapshot/rZrkEyj8jekTQEdQOB1aa376Z31QhnlT
jemalloc: https://grafana.tout.im/dashboard/snapshot/h26vrjclAA5pvvPJOe5EQhSGDRBii7dC

We can see that jemalloc doesn't slowly leak through fragmenting as the glibc one.
The memory spikes seems to stay under the maximum mem we can see for the glibc case at the end, so I would say it's fine ?

For any correlation to the CPU cost it's difficult to judge. I am guessing that malloc is low enough in term of CPU usage so if there is an increase, it wouldn't be really noticeable, but I can't prove it.

[anoadragon453]

@MatMaul This looks fine to me. @richvdh mentioned that it was likely that glibc is experiencing the same spikes, it's just that we're not getting the memory back immediately afterwards.

I think modifying it to be the default makes sense :)

[maquis196]

The advice in the readme;

-e LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so.2

That would be incorrect for the arm images though, ideally we want;

-e LD_PRELOAD=/usr/lib/uname -m-linux-gnu/libjemalloc.so.2

I assume that would on the docker cli, doubt it would for docker-compose though. (or just leave a note for arm users?)

[clokep]

Is this waiting on a decision? Wondering if we can unstick this.

[anoadragon453]

@maquis196 thank you, good catch.

@clokep We've decided on installing jemalloc in the docker image and setting it to be used by default. This involves adding a ENV LD_PRELOAD=/usr/lib/uname -m-linux-gnu/libjemalloc.so.2 line to the dockerfile, and modifying the documentation to inform users on how to not use jemalloc if they so wish. @MatMaul are you able to make these changes?

[MatMaul]

so the problem is that I can't use uname in ENV, that's why it is not already done ^^

https://vsupalov.com/docker-env-vars/
This page suggests to use an input ARG that you specify at docker build time.

I am trying to find a way to access the arch info at build time but for now I am failing.

Ideas welcome :)

[MatMaul]

I can do that at runtime in start.py however.

something like that (pseudocode):

jemallocpath = "/usr/lib/"+ arch + "-linux-gnu/libjemalloc.so.2"
if file(jemallocpath).exists:
  set_env_var(LD_PRELOAD, jemallocpath)

Thoughts ?

[anoadragon453]

Hmm, is it possible to simply use LD_PRELOAD=/usr/lib/*-linux-gnu/libjemalloc.so.2?

[dev-zero]

jemallocpath = "/usr/lib/"+ arch + "-linux-gnu/libjemalloc.so.2"
if file(jemallocpath).exists:
  set_env_var(LD_PRELOAD, jemallocpath)

I'd prefer that over the Dockerfile-hack since it allows for explicit path checking and logging.
@anoadragon453 should work if the shell form is used for the entrypoint (which currently isn't the case).

[anoadragon453]

Performing the check in /start.py sounds fine, as it is what starts Synapse anyways.

start.py uses os.execv for running Synapse though, which means we need to set the environment variable in a way os.execv will pick it up. Also note that os.execv will replace the running python process with whatever it runs.

@dev-zero what's the benefit of using the shell form?

[dev-zero]

@anoadragon453 the Dockerfile reference does only mention that other env vars will be substituted, but for ENV LD_PRELOAD=/usr/lib/*-linux-gnu/libjemalloc.so.2 to work you need full shell expansion (I don't think the loader will do it). The other place where this could presumably happen is if the entrypoint is run through sh -c. But I haven't tested any of this so far.

As for the os.execv: since it inherits the present os.environ, setting an env variable should work.
The other option would be to use execve instead and make it explicit:

extra_env = {}

if file(jemallocpath).exists:
  extra_env['LD_PRELOAD'] = jemallocpath
...
os.execve(..., dict(os.environ, **extra_env))

[anoadragon453]

Thanks for the detailed reply.

The other option would be to use execve instead and make it explicit

This sound a bit clearer to me, and ensures nothing else will be adversely affected.

[Half-Shot]

I've made the changes described above and checked that it works (builds/runs) fine on a x84_64 machine.

[erikjohnston]

Otherwise LGTM