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.

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

Add systemd memory note on install and in config #11641

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Feb 22, 2024

I got caught by this without noticing for a couple of weeks, wondering why my node was behaving so difficult when I tried to do anything non-trivial. I'm also wondering if raising this in general might be a good idea at this point since it seems to be pushing the limit with the size of the blockstore even with minimal history.

@rvagg rvagg requested a review from a team as a code owner February 22, 2024 04:52
Comment on lines 16 to 17
MemoryHigh=8G
MemoryMax=10G
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
MemoryHigh=8G
MemoryMax=10G
MemoryHigh=16G
MemoryMax=20G

🤷 my node was struggling to even stay in sync so it would be good to lift this at bit. Suggestions? 12/16 maybe as a compromise if we're still aiming for laptop installs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even just having a MemoryHigh is probably causing heavy performance degradation aside from the memory limitation since the process doesn't get to know this pressure is being applied except that allocation calls are slower and reclamation efforts are more aggressive. Ideally this would be removed entirely but it does get very hungry on some of the big API calls.

@rvagg
Copy link
Member Author

rvagg commented Feb 22, 2024

@f8-ptrk suggested on Slack that we just remove this entirely because they reflect ancient times when these limits were practical. I'm happy to do that, maybe leave a note in there with them commented out suggesting they can be used if it's too much of a hog. Thoughts?

@rjan90
Copy link
Contributor

rjan90 commented Feb 22, 2024

@f8-ptrk suggested on Slack that we just remove this entirely because they reflect ancient times when these limits were practical. I'm happy to do that, maybe leave a note in there with them commented out suggesting they can be used if it's too much of a hog. Thoughts?

Agree that it makes sense to get rid of these limits, and rather have them commented out with the suggestion that they can be set.

@rvagg
Copy link
Member Author

rvagg commented Feb 22, 2024

👍 done. I've removed the note from Makefile that I had in there, we have to assume people are running this and are fairly serious about allocating resources. I've left the Makefile print formatting though. Note left in the service config with those options commented out. I left the fd limits, I think they're probably high enough as they are unless someone thinks otherwise.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Makes sense. This will affect docker images but... users should apply memory limits to docker itself.

@Stebalien Stebalien merged commit 3a87b3e into master Feb 22, 2024
87 of 88 checks passed
@Stebalien Stebalien deleted the rvagg/systemd-notes branch February 22, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

3 participants