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

Enable Puma's nakayoshi_fork feature #11121

Merged
merged 2 commits into from Oct 28, 2020
Merged

Enable Puma's nakayoshi_fork feature #11121

merged 2 commits into from Oct 28, 2020

Conversation

benhalpern
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

gc.compact

Here are a few posts about GC.compact in Rails....

My thoughts here are that if tests pass, this is something we could deploy and observe. There may be more ways we could use this in specific cases.

@benhalpern benhalpern requested a review from a team as a code owner October 27, 2020 15:16
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 27, 2020
@krzysiek1507
Copy link
Contributor

Hi! If you deploy this, could you please share the stats?

@benhalpern
Copy link
Contributor Author

@krzysiek1507 absolutely. If folks feel like this is worth attempting a deploy, I think the immediate change in memory would 10000% be worth publishing. 😄

@krzysiek1507
Copy link
Contributor

That'd be great! :D

Have you considered https://fullstaqruby.org/?

@benhalpern
Copy link
Contributor Author

@krzysiek1507 that is very interesting.... And we'll definitely be looking more and more at this sort of stuff, as we want to ensure the cost-effectiveness of the Forem network. Most of our choices have been DEV-centric, and while we certainly cared about efficiency, it will be exciting to explore solutions now that resource management is much more central to our concerns.

.... So no, haven't really explored fullstaq in detail, but I think we'll be paying much more attention to innovations in this space going forward. I'd think @citizen428 has been paying closest attention to this stuff from our team.

Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

Tested locally and works well, why not give it a shot! 🚀

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 27, 2020
@rhymes
Copy link
Contributor

rhymes commented Oct 27, 2020

Seems like Puma 5 had added a similar experimental feature in its release in cluster mode, they are also inviting people to contribute data:

If you upgrade and try any of the 3 new features, please post before and after results or screenshots to this Github issue. “It didn’t do anything” is still a useful report in this case. Posting ~24 hours of “before” and ~24 hours of “after” data would be most helpful.

from https://www.speedshop.co/2020/09/17/we-made-puma-faster-with-sleep-sort.html

Maybe it's worth testing nakayoshi_fork and report back to puma/puma#2258 as we are a solid use case of a production Rails app with lots of traffic ?

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 27, 2020
@benhalpern
Copy link
Contributor Author

@rhymes Good call. I've experimented with the old gem, but this being the more straightforward use of GC.compact seems ideal.

I took some "before" screenshots.

@maestromac maestromac changed the title Run GC.compact before fork Run nakayoshi_fork before in puma Oct 27, 2020
@maestromac maestromac changed the title Run nakayoshi_fork before in puma Enable nakayoshi_fork in puma Oct 27, 2020
@maestromac maestromac changed the title Enable nakayoshi_fork in puma Enable Puma's nakayoshi_fork feature Oct 27, 2020
@citizen428
Copy link
Contributor

citizen428 commented Oct 28, 2020

@benhalpern Hm, I could have sworn I added this/recommend adding this way before I even joined the team... 😕 Anyway, 👍 from me.

@krzysiek1507 I've been following Fullstaq Ruby a bit. To me this seems like REE (Ruby Enterprise Edition) revamped, which I used way back in the day before that went EOL. It's certainly interesting, but we already use jemalloc, which means the only other things this offers is the malloc_trim patch. While that could certainly be useful, I'd hope that it or something like it finds its way into mainline Ruby if proven useful in the wild. Personally I'm much more interested in TruffleRuby, especially on Substrate VM.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 28, 2020
@benhalpern
Copy link
Contributor Author

Hm, I could have sworn I added this/recommend adding this way before I even joined the team... 😕 Anyway, 👍 from me.

@citizen428 We did go on this recommendation, but it didn't change much so we pulled it out to follow up with other experiments in the future. However, this was pre Ruby 2.7 when it was a gem and not something built into Puma with a couple lines of code, and a very different time in the project in terms of what the app does and how we configure it, so I suspect a better analysis.

So yeah, we followed the recommendation, but didn't really see much of a difference, so went with the the choice of being closer to defaults/conventions (all else equal).... Here's hoping this time we see more of an impact.

@benhalpern benhalpern merged commit 0847c38 into master Oct 28, 2020
@benhalpern benhalpern deleted the ben/gc-compact branch October 28, 2020 13:22
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 28, 2020
@nateberkopec
Copy link

Watching this to hear any impact you see from nakayoshi_fork.

which means the only other things this offers is the malloc_trim patch

as far as I remember, malloc_trim performs about as well as jemalloc.

@citizen428
Copy link
Contributor

Thanks @benhalpern, that clears it up, let's hope we see bigger improvements this time then.

@benhalpern
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants