Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use jemalloc to prevent memory bloating #15972

Merged
merged 13 commits into from
Sep 23, 2024
Merged

Use jemalloc to prevent memory bloating #15972

merged 13 commits into from
Sep 23, 2024

Conversation

top4ek
Copy link
Contributor

@top4ek top4ek commented Jun 25, 2024

Use dynamically linked jemalloc to prevent memory bloating in long living instances.

oliverguenther
oliverguenther requested changes Jul 5, 2024
View reviewed changes
Copy link
Member

@oliverguenther oliverguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @top4ek, thanks for your contribution and interest in improvements. We build multiple architectures for our dockerfiles, so this PR will fail them: https://github.com/opf/openproject/blob/dev/.github/workflows/docker.yml#L105-L127

/cc @opf/ops

@top4ek
Copy link
Contributor Author

top4ek commented Jul 5, 2024

Oh, thanks, will look into this.
Quick googling gives some problems with arm and ppc platforms so can we use it just for x86?
I can try to poke it with RPi4 later and build image accordingly for arm, but not ppc. I'm not sure we need to use unofficial jemalloc enabled ruby images.

@top4ek top4ek marked this pull request as draft July 5, 2024 17:19
@top4ek top4ek marked this pull request as ready for review July 8, 2024 13:43
@top4ek top4ek requested a review from oliverguenther July 8, 2024 13:43
@top4ek
Copy link
Contributor Author

top4ek commented Jul 8, 2024

Added support for other architectures. Using USE_JEMALLOC=false enviromnent by default.
I was unable to test it on my RPi4(low memory, it just hangs on image building). Also there's opened issues on arm and ppc architectures in jemalloc repo.
x86 is battle tested already for couple of weeks on OP and it's reduced memory usage at least twice. (My previous experience at different project with jemalloc allowed us to use 4 times more sidekiq instances on the same hardware limitations).
Other architectures MUST be tested, but I don't have access to them.

@oliverguenther oliverguenther requested review from crohr and machisuji and removed request for oliverguenther August 26, 2024 12:48
crohr
crohr reviewed Aug 26, 2024
View reviewed changes
docker/prod/setup/preinstall-common.sh Outdated
elif [ "$ARCHITECTURE" = "ppc64le" ]; then
LIB_ARCH="powerpc64le"
fi
export LD_PRELOAD=/usr/lib/${LIB_ARCH}-linux-gnu/libjemalloc.so.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@top4ek aren't we supposed to also set that env variable when launching a container from this image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looks like you're right. Just looked into fresh instance with this patch and it doesn't work. export is not set on build stage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now there would need to be an env variable basically saying "launch with jemalloc" (can reuse USE_JEMALLOC for that) that sets the LD_PRELOAD path in the entrypoint.

Copy link
Contributor Author

@top4ek top4ek Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hoped to bake LD_PRELOAD straight into the image on build stage to make it more "systemwide". Quick googling said that i can't do that inside dockerfile's RUN.
Got some ideas, will try them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the existing entrypoint.sh file, and simply export the relevant variables if USE_JEMALLOC env variable is set to true by the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but entrypoint.sh is for only all-in-one target, so I'm going to create additional entrypoint script for slim target and don't want to copy-paste same code in both scripts.

@machisuji
Copy link
Member

I've tested this on ppc64 and double-checked via

MALLOC_CONF=stats_print:true ruby -e "exit"
# prints
#   --- End jemalloc statistics ---
# at the end

to see if ruby is indeed using JEMALLOC which it is. So that's good.

So this PR is fine for x86 and ppc64 at least. And I reckon arm64 will be fine too. This can go ahead if we add the export LD_PRELOAD=... to the web (docker/prod/web) and worker (docker/prod/worker) scripts. Could you please amend that @top4ek?

machisuji
machisuji requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@machisuji machisuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@top4ek Thanks for the PR. I would actually set the default for USE_JEMALLOC to true.

The only thing that's missing is the LD_PRELOAD export in the web and worker scripts as described in my other comment. I would check for USE_JEMALLOC there too so that we can still choose not to use it.

@top4ek
Copy link
Contributor Author

top4ek commented Aug 27, 2024

LD_PRELOAD export in the web and worker scripts

The problem is that LD_PRELOAD is architecture dependent and it's can't be exported while building in RUN command. /etc/environment and /etc/profile(.d) don't cause any effect as per my tests.
I beleive that web and worker scripts are fine, but I'm playing with docker ENTRYPOINT scripts ATM to set variable on start via additionally generated script on preinstall stage.

USE_JEMALLOC to true

Not sure if it's safe to do without tests as because of jemalloc/jemalloc#2358

@top4ek top4ek marked this pull request as draft August 27, 2024 16:59
@top4ek top4ek marked this pull request as ready for review September 17, 2024 18:21
@top4ek
rubocop
05fea40
@top4ek
Merge branch 'use_jemalloc' of github.com:top4ek/openproject into use…
ecc7765 
…_jemalloc

* 'use_jemalloc' of github.com:top4ek/openproject: (1771 commits)
  [57927] Primerise the Notification badge in the top header (opf#16742)
  add test for WorkPackageEagerLoadingWrapper text formatting; addresses opf#16542 (comment)
  chore(deps): bump md-to-pdf to v0.1.2
  [57347] Fixed NextcloudConnectionValidator specs
  Remove special mobile scrolling behavior that we introduced to collapse the adress bar on iOS Safari. It somehow conflicts with the positioning of the ActionMenu
  [57347] Restructured NextcloudConnectionValidator
  [57347] Added test for failing files query on NextcloudConnectionValidator
  [57347] Added test for unexpected files validation for Nextcloud storage
  [57347] Added unexpected content validation to NextcloudConnectionValidator
  build(deps): bump fog-aws from 3.26.0 to 3.27.0
  build(deps-dev): bump rubocop-performance from 1.21.1 to 1.22.0
  add a trailing arrow-down icon for add button in meeting module
  update locales from crowdin [ci skip]
  update locales from crowdin [ci skip]
  update locales from crowdin [ci skip]
  update locales from crowdin [ci skip]
  update locales from crowdin [ci skip]
  [57911] Improve participants side panel phrasing and spacing
  [#53620] add feature flag for buit-in apps
  Add technical notes to lookbook forms page
  ...
@top4ek
Copy link
Contributor Author

top4ek commented Sep 17, 2024

Added entrypoint for slim container. Show jemalloc status at admin/info page.

machisuji
machisuji requested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@machisuji machisuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @top4ek, I think this looks good and works fine. I just have one suggestion. Could you please have a look?

docker/prod/Dockerfile Outdated Show resolved Hide resolved
machisuji
machisuji approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@machisuji machisuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@machisuji
Copy link
Member

Thanks for your contribution @top4ek! Merging it (into dev) now. That means it should become available officially with OpenProject 14.6 which is currently scheduled to be released in about two weeks.

@machisuji machisuji closed this Sep 23, 2024
@machisuji machisuji reopened this Sep 23, 2024
@machisuji machisuji merged commit f6e8038 into opf:dev Sep 23, 2024
17 checks passed
@top4ek top4ek deleted the use_jemalloc branch October 9, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverguenther oliverguenther oliverguenther requested changes

@machisuji machisuji machisuji approved these changes

@crohr crohr Awaiting requested review from crohr

Assignees
No one assigned
Labels
contribution docker needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@top4ek @machisuji @crohr @oliverguenther