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 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.
@nateberkopec nateberkopec mentioned this pull request May 11, 2020
1 of 1 task complete
@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 nateberkopec force-pushed the nakayoshi branch from 731a2f1 to 72ebf44 May 11, 2020
@nateberkopec
Copy link
Member Author

nateberkopec commented May 11, 2020

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.

@nateberkopec nateberkopec merged commit 2a2c3c7 into master May 11, 2020
51 of 53 checks passed
51 of 53 checks passed
ubuntu 2.2
Details
ubuntu 2.2
Details
build
Details
ubuntu 2.3
Details
ubuntu 2.3
Details
ubuntu 2.4
Details
ubuntu 2.4
Details
ubuntu 2.5
Details
ubuntu 2.5
Details
ubuntu 2.6
Details
ubuntu 2.6
Details
ubuntu 2.7
Details
ubuntu 2.7
Details
ubuntu head
Details
ubuntu head
Details
ubuntu jruby
Details
ubuntu jruby
Details
ubuntu truffleruby-head
Details
ubuntu truffleruby-head
Details
macos 2.2
Details
macos 2.2
Details
macos 2.3
Details
macos 2.3
Details
macos 2.4
Details
macos 2.4
Details
macos 2.5
Details
macos 2.5
Details
macos 2.6
Details
macos 2.6
Details
macos 2.7
Details
macos 2.7
Details
macos head
Details
macos head
Details
macos jruby macos jruby
Details
macos jruby
Details
macos truffleruby-head macos truffleruby-head
Details
macos truffleruby-head
Details
windows 2.2
Details
windows 2.2
Details
windows 2.3
Details
windows 2.3
Details
windows 2.4
Details
windows 2.4
Details
windows 2.5
Details
windows 2.5
Details
windows 2.6
Details
windows 2.6
Details
windows 2.7
Details
windows 2.7
Details
windows mingw
Details
windows mingw
Details
ubuntu jruby-head ubuntu jruby-head
Details
ubuntu jruby-head ubuntu jruby-head
Details
# 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)

This comment has been minimized.

Copy link
@schneems

schneems May 18, 2020

Contributor

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?

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec May 18, 2020

Author Member

Thaaaaat would be a bug

This comment has been minimized.

Copy link
@schneems

schneems May 19, 2020

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.