Skip to content

Conversation

@peterzhu2118
Copy link
Member

@peterzhu2118 peterzhu2118 commented Apr 6, 2021

I'm proposing to deprecate rb_gc_force_recycle and make it a no-op function because it is a burden to maintain and makes changes to the GC difficult. It is also easy to incorrectly use this function and cause memory leaks such as #18065.

@peterzhu2118 peterzhu2118 requested a review from ko1 April 6, 2021 17:52
@peterzhu2118 peterzhu2118 changed the title Remove rb_gc_force_recycle Deprecate rb_gc_force_recycle Nov 5, 2021
@peterzhu2118 peterzhu2118 force-pushed the pz-remove-rb-gc-force-recycle branch 4 times, most recently from a2ee0c4 to ebc2cc6 Compare November 5, 2021 15:18
Comment on lines 211 to 213
* @deprecated This is now a no-op function, use RB_GC_GUARD instead.
*/
RBIMPL_ATTR_DEPRECATED(("this is now a no-op function, use RB_GC_GUARD instead"))
Copy link
Member

Choose a reason for hiding this comment

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

The "use RB_GC_GUARD instead" advice can be unsought. People might not understand why they should use RB_GC_GUARD instead of it. In fact they don't have to, unless they are Eric Wong. It seems nobody except Eric could be creative enough to abuse this function as a GC guard.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I've updated the message to not include that suggestion.

This commit removes usages of rb_gc_force_recycle since it is a burden
to maintain and makes changes to the GC difficult.
@peterzhu2118 peterzhu2118 force-pushed the pz-remove-rb-gc-force-recycle branch from ebc2cc6 to 49dbf0a Compare November 8, 2021 13:42
gc.c Outdated
*/
}
RB_VM_LOCK_LEAVE();
RB_GC_GUARD(obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I don't think it's needed.

…mark_stack_chunk

This commit deprecates rb_gc_force_recycle and coverts it to a no-op
function. Also removes invalidate_mark_stack_chunk since only
rb_gc_force_recycle uses it.
@peterzhu2118 peterzhu2118 force-pushed the pz-remove-rb-gc-force-recycle branch from 49dbf0a to ba92b0d Compare November 8, 2021 17:57
@peterzhu2118 peterzhu2118 merged commit 3094064 into ruby:master Nov 8, 2021
@peterzhu2118 peterzhu2118 deleted the pz-remove-rb-gc-force-recycle branch November 8, 2021 19:05
@ko1
Copy link
Contributor

ko1 commented Nov 8, 2021

Could you add about this change on NEWS? (sorry I missed it before merging)

@peterzhu2118
Copy link
Member Author

@ko1 I opened #5095

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants