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

Added nakayoshi_fork option. #2256

Merged
merged 2 commits into from May 11, 2020
Merged

Added nakayoshi_fork option. #2256

merged 2 commits into from May 11, 2020

Conversation

nateberkopec
Copy link
Member

@nateberkopec nateberkopec commented May 11, 2020

Reduce memory usage in preloaded cluster-mode apps by GCing before
fork and compacting, where available.

Description

Please describe your pull request. Thank you for contributing! You're the best.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@jjb
Copy link
Contributor

jjb commented May 11, 2020

the nakayoshi gem does a max of 4 times, not sure if that's a theoretical ideal to also use here

Reduce memory usage in preloaded cluster-mode apps by GCing before
fork and compacting, where available.
@nateberkopec
Copy link
Member Author

True, changed. I'm going to leave out the full algorithm for now because a) license issues w/incorporating someone else's MIT-licensed algo that I don't feel like dealing with and b) compact implies GC, so we could probably GC fewer times in that case.

I want to punt on all the perf issues and possible optimizations until we figure out if this even decreases memory usage at all, if it does then we can deal with these issues.

# also increase time to boot and fork. See your logs for details on how much
# time this adds to your boot process. For most apps, it will be less than one
# second.
def nakayoshi_fork(enabled=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we defaulting this to false? It seems weird that adding this in my config does nothing:

nakayoshi_fork

I have to

nakayoshi_fork(true)

Was this intentional or can we change the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thaaaaat would be a bug

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe that’s why there was so little change in CodeTriage

@nateberkopec nateberkopec deleted the nakayoshi branch September 4, 2020 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants