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

run regular GC if compact not available #2228

Closed
wants to merge 2 commits into from
Closed

run regular GC if compact not available #2228

wants to merge 2 commits into from

Conversation

jjb
Copy link
Contributor

@jjb jjb commented Apr 21, 2020

Description

While doing some simple experiments with GC.compact in ruby 2.7 I noticed that
GC.start also had some benefits in the pre_fork phase. Since the master process won't be
growing (as much? at all?) as the forked processes, it might not ever get an organic GC, so this
is perhaps a good time to get in a manual one to save a meg or three.

(I actually don't know if the garbage collection done by GC.start reduces forked memory and/or CoW effeciency — if so, an even better idea to do it for all ruby versions).

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • [ x] 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.
  • [x ] I have added appropriate tests if this PR fixes a bug or adds a feature.
  • [ x] My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • [ x] If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • [x ] If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • [x ] I have updated the documentation accordingly.
  • [x ] All new and existing tests passed, including Rubocop. i did not run tests because i was unable to install ragel

@nateberkopec
Copy link
Member

I've been thinking about this. See nakayoshi_fork.

I think maybe we should:

if configured_to_cow_friendly_fork
3.times { GC.start }
if GC.respond_to?(:compact)
  GC.compact
end
end

This will introduce a lag time before forking, maybe up to 1-2 seconds. I'm not sure if that amount of lag would be acceptable for everyone so we may need to put the whole thing behind a config flag.

@nateberkopec
Copy link
Member

The "3 times" is very important here because you're trying to promote all existing objects to the old generation.

I'm actually not sure if it would be better to compact before or after that, because I don't know how generations interaction w/compaction.

@nateberkopec
Copy link
Member

If you try combinations of GC.start/GC.compact ordering, and test my thesis that 3 runs is better than 1, and track latency of all these options and report back, that would be 🙇 .

Maybe test against rubygems.org or another decently large open source app.

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Apr 21, 2020
@jjb
Copy link
Contributor Author

jjb commented Apr 21, 2020

fascinating, I didn't know that multiple runs of GC.start could make a difference. I guess ruby GC makes some sort of time/value compromise and doesn't always do the best it can?

in a test just now with my rails app, I set cache_classes and eager_load to true, and put binding.irb inside puma before_fork. i observed memory in Activity Monitor:

started at 288.3
after 1 GC.start: 279.1
subsequent GC.start did not change it
GC.compact did not change it! sad trombone
(but maybe it made the internal pages more efficient and better for forking/CoW?)

Then I did this test with 10 workers. I only ran each test once and I didn't put any load on the workers, so didn't really flex the CoW situation:

https://gist.github.com/jjb/d65bc10bb55fed7e4cafb07be035c10a

the original implementation of compact ran a GC before and after https://bugs.ruby-lang.org/issues/15626 I don't know if that made it into the final implementation ruby/ruby@3c55b64

@nateberkopec
Copy link
Member

guess ruby GC makes some sort of time/value compromise and doesn't always do the best it can?

In a way yes - this is the premise of a Generational GC.

In order for the test to be "fair" you'll have to put some load on your server after forking. The premise of nakayoshi_fork and compact-before-fork is that you improve copy-on-write by putting old objects all together on the fewest number of pages/space possible, which means you will do more copying and less writing as the server process takes requests.

@jjb
Copy link
Contributor Author

jjb commented Apr 21, 2020

In a way yes - this is the premise of a Generational GC.

ah, gotcha

In order for the test to be "fair" you'll have to put some load on your server after forking

alas I don't have time right now to set something up. would be cool to do a test with https://github.com/noahgibbs/rsb/

@nateberkopec
Copy link
Member

Adding contrib-wanted if anyone wants to try to run those tests themselves.

@nateberkopec
Copy link
Member

I'm thinking about putting this + compact behind an option for 5.0 and gathering data that way.

@jjb
Copy link
Contributor Author

jjb commented May 11, 2020

sounds good to me!

@nateberkopec
Copy link
Member

Closing in favor of #2256

@jjb jjb deleted the patch-2 branch October 5, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contrib-wanted feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants