Expose need_major_gc via GC.latest_gc_info #6791
Conversation
|
@tenderlove @peterzhu2118 - I see you've done GC-related work recently. Would you be able to review this, provide feedback, or route to someone else who could take a look? We've been running this at Stripe for a while and would love to get it upstream so that we can remove our custom patch in the future |
| @@ -11020,6 +11022,19 @@ gc_info_decode(rb_objspace_t *objspace, const VALUE hash_or_key, const unsigned | |||
| Qnil; | |||
| SET(major_by, major_by); | |||
|
|
|||
| if (orig_flags == 0) { /* set need_major_by only if flags not set explicitly */ | |||
There was a problem hiding this comment.
Since need_major_flags is only needed inside the if block, we can move it in.
| if (orig_flags == 0) { /* set need_major_by only if flags not set explicitly */ | |
| if (orig_flags == 0) { /* set need_major_by only if flags not set explicitly */ | |
| unsigned int need_major_flags = objspace->rgengc.need_major_gc; | |
10d3ebe to
38f1b7d
Compare
|
@peterzhu2118 thanks for looking! Moved |
|
Sorry I missed it. I feel strange because I don't against this feature, but I think it was better that the name implies next GC's information. |
|
@ko1 I agree it's a bit strange - I commented on it in the description:
I don't feel strongly about the name - happy to change it if you have a better suggestion! I like |
|
On the other hand I don't think it is strange in
I agree about it. But internal naming can be changed easily, but exposed naming is difficult to change (for example, if we will change it on Ruby 3.3, your tool depend on it will be broken). This is why we are discussing naming long time. Anyway, it is too late so I left it. Next time, please ask me or please file it on bugs.ruby-lang.org and Matz will decide. |
We trigger GC out-of-band to reduce latency impact on synchronous request processing. It’s not perfect, but it works. One thing we really want to avoid is major GC during request processing - so we don’t want to start request processing in a state where
need_major_gcis set. If there is a risk of major GC, we prefer to run it out-of-band, where it doesn't matter for request latency.Unfortunately, this state is currently not exposed - so we can’t know whether the next GC is set to be major or not. As a workaround, we trigger minor GC twice in a row - if a major GC threshold is hit during the first GC, the second GC is upgraded to major and there is no immediate risk of another major GC. This mostly works, but it's not great and it's not efficient (the second GC is not cheap and it's unnecessary in most cases); this PR makes it possible to do that in a better way by exposing
need_major_gc.It may seem strange to expose it under
GC.latest_gc_infobut I think it’s a reasonable place because the decision was made during the latest GC.